| Message ID | 20211020084315.2687562-1-dominique.martinet@atmark-techno.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | archive_handler: fail update on more archive errors | expand |
Hi Dominique, On 20.10.21 10:43, Dominique Martinet wrote: > Do not mark update as successful if archive could not be extracted! > > Current archive handler does not fail update when FS is full and cannot > be written to after archive_write_header() or archive_write_data_block(). > > libarchive is nice enough to return ARCHIVE_WARN when the error is not > fatal, but unfortunately inability to write a data block is returned as > a warning so the write functions treat anything non-OK as an error. > Ouch...this is not what I assumed when I implemented the handler. > Trust warning status for read functions and only print a message in > these case as we used to do. > While we are here add entry name to error message when it makes sense. > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > > Stumbled on this on testing and was surprised swupdate didn't fail the > install, as I rebooted into a filesystem with some empty or missing > files... Very bad... > > I honestly have no idea what is warning and what isn't -- when I > reproduced this for this patch, failing to create a file returned as > ARCHIVE_ERROR so complete failure, but failing to append to an existing > file is apparently only a warning, this does not make sense to me... This does not make sense for me, too, and then SWUpdate does not forward the error. > > I also couldn't get it to error anything in case of mode application > failure (e.g. trying to extract an archive on a fat filesystem), but at > least this mean that I didn't break this usecase -- that'll still work > as before. > > > Thanks, > Dominique > > handlers/archive_handler.c | 46 ++++++++++++++++++++++++-------------- > 1 file changed, 29 insertions(+), 17 deletions(-) > > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c > index 8ee69ffdba78..ced7554125b1 100644 > --- a/handlers/archive_handler.c > +++ b/handlers/archive_handler.c > @@ -40,7 +40,7 @@ struct extract_data { > }; > > static int > -copy_data(struct archive *ar, struct archive *aw) > +copy_data(struct archive *ar, struct archive *aw, struct archive_entry *entry) > { > int r; > const void *buff; > @@ -55,12 +55,20 @@ copy_data(struct archive *ar, struct archive *aw) > r = archive_read_data_block(ar, &buff, &size, &offset); > if (r == ARCHIVE_EOF) > return (ARCHIVE_OK); > - if (r != ARCHIVE_OK) > - return (r); > + if (r != ARCHIVE_OK) { > + if (r == ARCHIVE_WARN) { > + WARN("archive_read_next_header(): %s for '%s'", > + archive_error_string(ar), archive_entry_pathname(entry)); > + } else { > + ERROR("archive_read_data_block(): %s for '%s'", > + archive_error_string(ar), archive_entry_pathname(entry)); > + return (r); > + } > + } > r = archive_write_data_block(aw, buff, size, offset); > if (r != ARCHIVE_OK) { > - TRACE("archive_write_data_block(): %s", > - archive_error_string(aw)); > + ERROR("archive_write_data_block(): %s for '%s'", > + archive_error_string(aw), archive_entry_pathname(entry)); > return (r); > } > } > @@ -141,8 +149,8 @@ extract(void *p) > if (r != ARCHIVE_OK) { > if (r == ARCHIVE_EOF) > break; > - if (r == ARCHIVE_WARN ) { > - WARN("archive_read_next_header(): %s '%s'", > + if (r == ARCHIVE_WARN) { > + WARN("archive_read_next_header(): %s for '%s'", > archive_error_string(a), archive_entry_pathname(entry)); > } else { > ERROR("archive_read_next_header(): %s", > @@ -155,17 +163,21 @@ extract(void *p) > TRACE("Extracting %s", archive_entry_pathname(entry)); > > r = archive_write_header(ext, entry); > + if (r != ARCHIVE_OK) { > + ERROR("archive_write_header(): %s", > + archive_error_string(ext)); > + goto out; > + } > + > + r = copy_data(a, ext, entry); > if (r != ARCHIVE_OK) > - TRACE("archive_write_header(): %s", > - archive_error_string(ext)); > - else { > - copy_data(a, ext); > - r = archive_write_finish_entry(ext); > - if (r != ARCHIVE_OK) { > - ERROR("archive_write_finish_entry(): %s", > - archive_error_string(ext)); > - goto out; > - } > + goto out; /* warning already printed in copy_data() */ > + > + r = archive_write_finish_entry(ext); > + if (r != ARCHIVE_OK) { > + ERROR("archive_write_finish_entry(): %s for '%s'", > + archive_error_string(ext), archive_entry_pathname(entry)); > + goto out; > } > > } > Acked-by: Stefano Babic <sbabic@denx.de> Best regards, Stefano Babic
diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c index 8ee69ffdba78..ced7554125b1 100644 --- a/handlers/archive_handler.c +++ b/handlers/archive_handler.c @@ -40,7 +40,7 @@ struct extract_data { }; static int -copy_data(struct archive *ar, struct archive *aw) +copy_data(struct archive *ar, struct archive *aw, struct archive_entry *entry) { int r; const void *buff; @@ -55,12 +55,20 @@ copy_data(struct archive *ar, struct archive *aw) r = archive_read_data_block(ar, &buff, &size, &offset); if (r == ARCHIVE_EOF) return (ARCHIVE_OK); - if (r != ARCHIVE_OK) - return (r); + if (r != ARCHIVE_OK) { + if (r == ARCHIVE_WARN) { + WARN("archive_read_next_header(): %s for '%s'", + archive_error_string(ar), archive_entry_pathname(entry)); + } else { + ERROR("archive_read_data_block(): %s for '%s'", + archive_error_string(ar), archive_entry_pathname(entry)); + return (r); + } + } r = archive_write_data_block(aw, buff, size, offset); if (r != ARCHIVE_OK) { - TRACE("archive_write_data_block(): %s", - archive_error_string(aw)); + ERROR("archive_write_data_block(): %s for '%s'", + archive_error_string(aw), archive_entry_pathname(entry)); return (r); } } @@ -141,8 +149,8 @@ extract(void *p) if (r != ARCHIVE_OK) { if (r == ARCHIVE_EOF) break; - if (r == ARCHIVE_WARN ) { - WARN("archive_read_next_header(): %s '%s'", + if (r == ARCHIVE_WARN) { + WARN("archive_read_next_header(): %s for '%s'", archive_error_string(a), archive_entry_pathname(entry)); } else { ERROR("archive_read_next_header(): %s", @@ -155,17 +163,21 @@ extract(void *p) TRACE("Extracting %s", archive_entry_pathname(entry)); r = archive_write_header(ext, entry); + if (r != ARCHIVE_OK) { + ERROR("archive_write_header(): %s", + archive_error_string(ext)); + goto out; + } + + r = copy_data(a, ext, entry); if (r != ARCHIVE_OK) - TRACE("archive_write_header(): %s", - archive_error_string(ext)); - else { - copy_data(a, ext); - r = archive_write_finish_entry(ext); - if (r != ARCHIVE_OK) { - ERROR("archive_write_finish_entry(): %s", - archive_error_string(ext)); - goto out; - } + goto out; /* warning already printed in copy_data() */ + + r = archive_write_finish_entry(ext); + if (r != ARCHIVE_OK) { + ERROR("archive_write_finish_entry(): %s for '%s'", + archive_error_string(ext), archive_entry_pathname(entry)); + goto out; } }
Do not mark update as successful if archive could not be extracted! Current archive handler does not fail update when FS is full and cannot be written to after archive_write_header() or archive_write_data_block(). libarchive is nice enough to return ARCHIVE_WARN when the error is not fatal, but unfortunately inability to write a data block is returned as a warning so the write functions treat anything non-OK as an error. Trust warning status for read functions and only print a message in these case as we used to do. While we are here add entry name to error message when it makes sense. Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- Stumbled on this on testing and was surprised swupdate didn't fail the install, as I rebooted into a filesystem with some empty or missing files... I honestly have no idea what is warning and what isn't -- when I reproduced this for this patch, failing to create a file returned as ARCHIVE_ERROR so complete failure, but failing to append to an existing file is apparently only a warning, this does not make sense to me... I also couldn't get it to error anything in case of mode application failure (e.g. trying to extract an archive on a fat filesystem), but at least this mean that I didn't break this usecase -- that'll still work as before. Thanks, Dominique handlers/archive_handler.c | 46 ++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 17 deletions(-)