[pacman-dev] [PATCH] Turn libarchive warnings into libalpm warnings
Rather than hiding these warnings, show them to the user as they happen.
This will prevent things such as hiding full filesystem errors (ENOSPC) from
the user as seen in FS#11639.
Signed-off-by: Dan McGee
This should hopefully address some of the concerns raised in FS#11639 with
regards to continuing after filling the disk up. I'm not sure if we are
being too quick on the trigger here by failing after the package on every
warning libarchive can emit, but it remains to be seen.
Add some more checks and passing of error conditions between our functions.
We now ensure errors are returned when warnings or errors occur extracting a
single file, and then note the presence of errors after extraction of an
entire package is complete. If so, we abort the transaction to be on the
safe side and keep damage to a minimum.
Signed-off-by: Dan McGee
Hey guys,
I would *love* it if someone could take this patch and run with it. I really just wanted to hash out a possible fix, but unfortunately it isn't easy for two reasons: our code structure is a bit rough, and we really don't have a hard-and-fast set of rules for continuing or failing immediately. If anyone wants to attack the problem further, you are more than welcome. Although this patch doesn't cause any pactests to fail, I'm a bit wary of applying it as-is, especially to maint.
-Dan
1. The first part of the patch is just a code clean-up, right? 2. The add_commit part: There is something I don't understand: We have trans->state and handle->trans->state. This is intentional? Probably you wanted to set handle->trans->state. (handle->trans != trans in case of sync transaction.) Btw, our transaction system is a nightmare, we should have *one* transaction at every moment... I don't know if this immediate stop is better. If we choose this way, we should give a BIG warning: Some packages were installed some packages were not. So your system probably became inconsistent (broken dependencies etc.) Bye P.S.: Transaction rollback would be the ideal solution. (Which is not an easy game.)
On Sat, Nov 1, 2008 at 7:58 AM, Nagy Gabor
Hey guys,
I would *love* it if someone could take this patch and run with it. I really just wanted to hash out a possible fix, but unfortunately it isn't easy for two reasons: our code structure is a bit rough, and we really don't have a hard-and-fast set of rules for continuing or failing immediately. If anyone wants to attack the problem further, you are more than welcome. Although this patch doesn't cause any pactests to fail, I'm a bit wary of applying it as-is, especially to maint.
-Dan
1. The first part of the patch is just a code clean-up, right? Yes, so we don't do the actual extraction in two different places. But it has important consequences because we now handle the two cases differently.
2. The add_commit part: There is something I don't understand: We have trans->state and handle->trans->state. This is intentional? Probably you wanted to set handle->trans->state. No, if things are sane, I want to abort the *current* transaction. We are looking at that one's packages. In this case, because this is the add transaction, we should only have one? Or am I completely off and we still make another sub-transaction.
(handle->trans != trans in case of sync transaction.) Btw, our transaction system is a nightmare, we should have *one* transaction at every moment... You are saying the same thing I've been thinking for a while. It is a huge mess and I'm not happy about it, so I'm right with you here. :)
I don't know if this immediate stop is better. If we choose this way, http://bugs.archlinux.org/task/11639 needs to get addressed, and preferably in a maint release. I'm all ears for other suggestions.
we should give a BIG warning: Some packages were installed some packages were not. So your system probably became inconsistent (broken dependencies etc.) The idea was your system is probably inconsistant because you have broken packages installed (files didn't make it out of the archive properly).
Bye
P.S.: Transaction rollback would be the ideal solution. (Which is not an easy game.) We've been saying this for a while too. :)
There is something I don't understand: We have trans->state and handle->trans->state. This is intentional? Probably you wanted to set handle->trans->state. No, if things are sane, I want to abort the *current* transaction. We are looking at that one's packages. In this case, because this is the add transaction, we should only have one? Or am I completely off and we still make another sub-transaction.
I may completely misunderstand something. So I guess that the purpose of this patch, that pacman should stop immediately after package upgrade if error occurs (not trying to commit the whole transaction, ie. installing the remaining packages.) This will work with -A/-U, where trans == handle->trans. However, if trans != handle->trans, setting trans->state has no effect, because add_commit watches the state of handle->trans only (in the first line of the loop). This is the situation with -S: handle->trans is a sync transaction, but sync_commit will create a new upgrade transaction, and it will commit it (this is passed to add_commit). So as I see, this patch has no effect on sync transaction. Maybe you forgot to insert a "break;" somewhere... Bye
participants (3)
-
Dan McGee
-
Dan McGee
-
Nagy Gabor