| Message ID | 20210713120059.19476-1-mike.looijmans@topic.nl |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Improve error handling around archive_read_next_header | expand |
Hi Mike, On 13.07.21 14:00, Mike Looijmans wrote: > ARCHIVE_OK is the most likely result of the method, so make that code path the fastest. > > archive_read_next_header may return ARCHIVE_WARN, which is not fatal and extracting can continue. > In particular, the message "Pathname can't be converted from UTF-8 to current locale" is of the > warning category only. The pathname will just be interpreted as a string of bytes, which is in > most cases just fine. > > In case of error, get rid of the extra "1" appended to the error message. > > This fixes that SWUpdate aborts when for example "ca-certificates" is added to the image inside the > tar file. Some of the filenames in that package have non-ASCII characters. > It makes absolutely sense. > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > --- > handlers/archive_handler.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c > index 9812d42..bafee80 100644 > --- a/handlers/archive_handler.c > +++ b/handlers/archive_handler.c > @@ -138,12 +138,17 @@ extract(void *p) > } > for (;;) { > r = archive_read_next_header(a, &entry); > - if (r == ARCHIVE_EOF) > - break; > if (r != ARCHIVE_OK) { > - ERROR("archive_read_next_header(): %s %d", > - archive_error_string(a), 1); > - goto out; > + if (r == ARCHIVE_EOF) > + break; > + if (r == ARCHIVE_WARN ) { > + WARN("archive_read_next_header(): %s '%s'", > + archive_error_string(a), archive_entry_pathname(entry)); > + } else { > + ERROR("archive_read_next_header(): %s", > + archive_error_string(a)); > + goto out; > + } > } > > if (debug) > It looks fine to me. Acked-by: Stefano Babic <sbabic@denx.de> Best regards, Stefano Babic
(offtopic: grmbl, google flagged this as spam for some reason...
Might be SPF failure? if you can do something about it:
X-Original-Authentication-Results: gmr-mx.google.com;
spf=fail (google.com: domain of mike.looijmans@topic.nl does not
designate 64.69.218.83 as permitted sender)
smtp.mailfrom=mike.looijmans@topic.nl
)
Mike Looijmans wrote on Tue, Jul 13, 2021 at 02:00:59PM +0200:
> ARCHIVE_OK is the most likely result of the method, so make that code path the fastest.
>
> archive_read_next_header may return ARCHIVE_WARN, which is not fatal and extracting can continue.
> In particular, the message "Pathname can't be converted from UTF-8 to current locale" is of the
> warning category only. The pathname will just be interpreted as a string of bytes, which is in
> most cases just fine.
>
> In case of error, get rid of the extra "1" appended to the error message.
>
> This fixes that SWUpdate aborts when for example "ca-certificates" is added to the image inside the
> tar file. Some of the filenames in that package have non-ASCII characters.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> handlers/archive_handler.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c
> index 9812d42..bafee80 100644
> --- a/handlers/archive_handler.c
> +++ b/handlers/archive_handler.c
> @@ -138,12 +138,17 @@ extract(void *p)
> }
> for (;;) {
> r = archive_read_next_header(a, &entry);
> - if (r == ARCHIVE_EOF)
> - break;
> if (r != ARCHIVE_OK) {
> - ERROR("archive_read_next_header(): %s %d",
> - archive_error_string(a), 1);
> - goto out;
> + if (r == ARCHIVE_EOF)
> + break;
> + if (r == ARCHIVE_WARN ) {
> + WARN("archive_read_next_header(): %s '%s'",
> + archive_error_string(a), archive_entry_pathname(entry));
> + } else {
> + ERROR("archive_read_next_header(): %s",
> + archive_error_string(a));
Would it make sense to include archive_entry_pathname(entry) in the
error message as well?
I don't see why it'd only work for warnings, and might be useful if it's
valid.
(Looking at archive.h header there's also ARCHIVE_RETRY that says "Retry
might succeed", I'm not sure in which case it happens so probably safe
to ignore until someone actually needs it)
That aside, looks good to me:
Reviewed-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c index 9812d42..bafee80 100644 --- a/handlers/archive_handler.c +++ b/handlers/archive_handler.c @@ -138,12 +138,17 @@ extract(void *p) } for (;;) { r = archive_read_next_header(a, &entry); - if (r == ARCHIVE_EOF) - break; if (r != ARCHIVE_OK) { - ERROR("archive_read_next_header(): %s %d", - archive_error_string(a), 1); - goto out; + if (r == ARCHIVE_EOF) + break; + if (r == ARCHIVE_WARN ) { + WARN("archive_read_next_header(): %s '%s'", + archive_error_string(a), archive_entry_pathname(entry)); + } else { + ERROR("archive_read_next_header(): %s", + archive_error_string(a)); + goto out; + } } if (debug)
ARCHIVE_OK is the most likely result of the method, so make that code path the fastest. archive_read_next_header may return ARCHIVE_WARN, which is not fatal and extracting can continue. In particular, the message "Pathname can't be converted from UTF-8 to current locale" is of the warning category only. The pathname will just be interpreted as a string of bytes, which is in most cases just fine. In case of error, get rid of the extra "1" appended to the error message. This fixes that SWUpdate aborts when for example "ca-certificates" is added to the image inside the tar file. Some of the filenames in that package have non-ASCII characters. Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- handlers/archive_handler.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)