Arch Linux

Permissions checking in the codebase is currently not the cleanest. This fix would be quite extreme for pacman 3.0.1 wrt testing any regressions, so I don't want to make it there. We really need an overhaul of permissions stuff before we can fix this.

What's the status of this?
Permissions checking isn't easy in general, but in relation to the current bug report, is there really a big problem ?
If -y is specified, then root access is needed, and that check should be enough.
And then maybe just moving the -y part at the top of the pacman_sync function.

Committed changes in my working branch to all -y to function with -i, -g, -l, and -s options.
e.g. -Syl -Sys, etc etc

Hmm, I see, it's more complicated than I thought.
I'm not really satisfied by this solution of calling needs_transaction() twice.
But I've yet to come up with a better solution.

Also, note that there are 2 problems with the cleanup part now :
706 cleanup:
707 |_if(data) {
708 |_|_alpm_list_free(data);
709 |_}
710 |_if(alpm_trans_release() == -1) {
711 |_|_fprintf(stderr, _("error: failed to release transaction (%s)\n"),
712 |_|_ alpm_strerrorlast());
713 |_|_retval = 1;
714 |_}

For the data check to work correctly, I had to move its declaration at top of pacman_sync function, and initialize it to NULL.
Otherwise, pacman segfaults on _alpm_list_free(data);.
Just try running : sudo pacman -Sys foo

After that, there is still a problem with the second part, when doing for example : pacman -Ss foo
In this case, pacman doesn't initialize a transaction (just like before), but it'll now try to release it, and fail.

needs_transaction() isn't an expensive function. In fact, it's a handful of bitwise operations and probably faster than function call overhead.

Calling it twice isn't a bad thing at all, and that was actually why i broke it out, because in cases like this the _only_ thing we can do is check this, or keep track of it in a variable.

There isn't much else we can do, we need to have knowledge of whether we need a transaction or not with the current state of things - in the long run, transactions shouldn't be managed on the front-end, but that's a LONG time from now.

Another problem with this code :
418 if(needs_transaction() &&
419 alpm_trans_init(PM_TRANS_TYPE_SYNC, config->flags, cb_trans_evt,

If pacman is run in a chroot (eg like pactest does), being root is not needed, it can be run as user.
So needs_transaction return false (even though a transaction is in fact needed), and the transaction is not initialized.

Just look at the number of pactests failing in make check.

But needs_transaction doesn't check the uid at all.

But it does this :
55 || (strcmp(alpm_option_get_root(), "/") != 0)) {

Ah, ok, that's not a check for the root _user_, that's a check for the root _path_.

And I see what you mean. I will fix that tonight

For this last problem, I just moved the root path check out of needs_transaction, and put it directly in pacman.c . I think this part is alright.

For the other problems, I thought about doing the transaction first, in a new sync trans function, which will init and release a transaction.
And then doing the commands like -Ss / -Sl / -Sg / -Si.

The problem is that for commands like -Sys / -Syl / etc, only the refresh part of the transaction should be done.
So I had to introduce an ugly sync_only hack.

Anyway, maybe this patch can give you some ideas.. But if you need more time to understand the crap I did than doing what you had in mind, just ignore it :)

Whoops, sorry, let me tag this one to do tonight.... /me runs in circles

Fixed the borken-ness, courtesy of the patch above.

http://code.phraktured.net/?p=pacman.git;a=shortlog;h=working