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 <dan@archlinux.org> --- 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 lib/libalpm/add.c | 89 +++++++++++++++++++++++++++++++++------------------- 1 files changed, 56 insertions(+), 33 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index f4caa5c..ee0898c 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -268,6 +268,32 @@ static int upgrade_remove(pmpkg_t *oldpkg, pmpkg_t *newpkg, pmtrans_t *trans, pm return(0); } +static int perform_extraction(struct archive *archive, + struct archive_entry *entry, const char *filename, const char *origname) +{ + int ret; + const int archive_flags = ARCHIVE_EXTRACT_OWNER | + ARCHIVE_EXTRACT_PERM | + ARCHIVE_EXTRACT_TIME; + + archive_entry_set_pathname(entry, filename); + + ret = archive_read_extract(archive, entry, archive_flags); + if(ret == ARCHIVE_WARN) { + /* operation succeeded but a non-critical error was encountered */ + _alpm_log(PM_LOG_WARNING, "warning extracting %s (%s)\n", + origname, archive_error_string(archive)); + return(2); + } else if(ret != ARCHIVE_OK) { + _alpm_log(PM_LOG_ERROR, _("could not extract %s (%s)\n"), + origname, archive_error_string(archive)); + alpm_logaction("error: could not extract %s (%s)\n", + origname, archive_error_string(archive)); + return(1); + } + return(0); +} + static int extract_single_file(struct archive *archive, struct archive_entry *entry, pmpkg_t *newpkg, pmpkg_t *oldpkg, pmtrans_t *trans, pmdb_t *db) @@ -278,9 +304,6 @@ static int extract_single_file(struct archive *archive, int needbackup = 0, notouch = 0; char *hash_orig = NULL; char *entryname_orig = NULL; - const int archive_flags = ARCHIVE_EXTRACT_OWNER | - ARCHIVE_EXTRACT_PERM | - ARCHIVE_EXTRACT_TIME; int errors = 0; entryname = archive_entry_pathname(entry); @@ -439,18 +462,13 @@ static int extract_single_file(struct archive *archive, int ret; snprintf(checkfile, PATH_MAX, "%s.paccheck", filename); - archive_entry_set_pathname(entry, checkfile); - - ret = archive_read_extract(archive, entry, archive_flags); - if(ret == ARCHIVE_WARN) { - /* operation succeeded but a non-critical error was encountered */ - _alpm_log(PM_LOG_WARNING, "warning extracting %s (%s)\n", - entryname_orig, archive_error_string(archive)); - } else if(ret != ARCHIVE_OK) { - _alpm_log(PM_LOG_ERROR, _("could not extract %s (%s)\n"), - entryname_orig, archive_error_string(archive)); - alpm_logaction("error: could not extract %s (%s)\n", - entryname_orig, archive_error_string(archive)); + + ret = perform_extraction(archive, entry, checkfile, entryname_orig); + if(ret == 2) { + /* warning */ + errors++; + } else if(ret == 1) { + /* error */ FREE(hash_orig); FREE(entryname_orig); return(1); @@ -589,18 +607,12 @@ static int extract_single_file(struct archive *archive, unlink(filename); } - archive_entry_set_pathname(entry, filename); - - ret = archive_read_extract(archive, entry, archive_flags); - if(ret == ARCHIVE_WARN) { - /* operation succeeded but a non-critical error was encountered */ - _alpm_log(PM_LOG_WARNING, "warning extracting %s (%s)\n", - entryname_orig, archive_error_string(archive)); - } else if(ret != ARCHIVE_OK) { - _alpm_log(PM_LOG_ERROR, _("could not extract %s (%s)\n"), - entryname_orig, archive_error_string(archive)); - alpm_logaction("error: could not extract %s (%s)\n", - entryname_orig, archive_error_string(archive)); + ret = perform_extraction(archive, entry, filename, entryname_orig); + if(ret == 2) { + /* warning */ + errors++; + } else if(ret == 1) { + /* error */ FREE(entryname_orig); return(1); } @@ -851,6 +863,7 @@ cleanup: int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) { int pkg_count, pkg_current; + int skip_ldconfig = 0, ret = 0; alpm_list_t *targ; ALPM_LOG_FUNC; @@ -868,19 +881,29 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) /* loop through our package list adding/upgrading one at a time */ for(targ = trans->packages; targ; targ = targ->next) { if(handle->trans->state == STATE_INTERRUPTED) { - return(0); + return(ret); } pmpkg_t *newpkg = (pmpkg_t *)targ->data; - commit_single_pkg(newpkg, pkg_current, pkg_count, trans, db); + if(commit_single_pkg(newpkg, pkg_current, pkg_count, trans, db)) { + /* something screwed up on the commit, abort the trans */ + trans->state = STATE_INTERRUPTED; + pm_errno = PM_ERR_TRANS_ABORT; + /* running ldconfig at this point could possibly screw system */ + skip_ldconfig = 1; + ret = -1; + } + pkg_current++; } - /* run ldconfig if it exists */ - _alpm_log(PM_LOG_DEBUG, "running \"ldconfig -r %s\"\n", handle->root); - _alpm_ldconfig(handle->root); + if(!skip_ldconfig) { + /* run ldconfig if it exists */ + _alpm_log(PM_LOG_DEBUG, "running \"ldconfig -r %s\"\n", handle->root); + _alpm_ldconfig(handle->root); + } - return(0); + return(ret); } /* vim: set ts=2 sw=2 noet: */ -- 1.6.0.3