diff mbox series

[v2,1/1] eal: update xz read support and ignore warning

Message ID 20230926133006.31534-1-syalavarthi@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2,1/1] eal: update xz read support and ignore warning |

Checks

Context Check Description
ci/loongarch-compilation success Compilation OK
ci/checkpatch warning coding style issues
ci/loongarch-unit-testing success Unit Testing PASS

Commit Message

Srikanth Yalavarthi Sept. 26, 2023, 1:30 p.m. UTC 
  archive_read_support_filter_xz returns a warning when
compression is not fully supported and is supported
through external program. This warning can be ignored
when reading the files through firmware open as only
decompression is required.

Fixes: 40edb9c0d36b ("eal: handle compressed firmware")
Cc: stable@dpdk.org

Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
Change-Id: I38cce556ec637af03dbde74d7d18318af48082d6
---
 lib/eal/unix/eal_firmware.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

Comments

David Marchand Sept. 26, 2023, 1:56 p.m. UTC | #1 
On Tue, Sep 26, 2023 at 3:30 PM Srikanth Yalavarthi
<syalavarthi@marvell.com> wrote:
>
> archive_read_support_filter_xz returns a warning when
> compression is not fully supported and is supported
> through external program. This warning can be ignored
> when reading the files through firmware open as only
> decompression is required.

Same comment as before, this sentence is confusing.
Compressing files does not matter in DPDK rte_firmware_ API.

The commit title and commitlog won't help people understand in which
case the issue is reproduced, and how this patch fixes it.

Suggesting the following title and commitlog:

"""
eal/unix: fix firmware reading with external xz support

libarchive may support xz decompression only through an external
(slower) helper.
In such a case, archive_read_support_filter_xz() returns ARCHIVE_WARN.

Fixes: 40edb9c0d36b ("eal: handle compressed firmware")
Cc: stable@dpdk.org
"""

>
> Fixes: 40edb9c0d36b ("eal: handle compressed firmware")
> Cc: stable@dpdk.org
>
> Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> Change-Id: I38cce556ec637af03dbde74d7d18318af48082d6

This is internal scm stuff and must be dropped.

> ---
>  lib/eal/unix/eal_firmware.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/lib/eal/unix/eal_firmware.c b/lib/eal/unix/eal_firmware.c
> index d1616b0bd9..16690b4245 100644
> --- a/lib/eal/unix/eal_firmware.c
> +++ b/lib/eal/unix/eal_firmware.c
> @@ -25,19 +25,31 @@ static int
>  firmware_open(struct firmware_read_ctx *ctx, const char *name, size_t blocksize)
>  {
>         struct archive_entry *e;
> +       int err;
>
>         ctx->a = archive_read_new();
>         if (ctx->a == NULL)
>                 return -1;
> -       if (archive_read_support_format_raw(ctx->a) != ARCHIVE_OK ||
> -                       archive_read_support_filter_xz(ctx->a) != ARCHIVE_OK ||
> -                       archive_read_open_filename(ctx->a, name, blocksize) != ARCHIVE_OK ||
> -                       archive_read_next_header(ctx->a, &e) != ARCHIVE_OK) {
> -               archive_read_free(ctx->a);
> -               ctx->a = NULL;
> -               return -1;
> -       }
> +
> +       if (archive_read_support_format_raw(ctx->a) != ARCHIVE_OK)
> +               goto error;
> +
> +       err = archive_read_support_filter_xz(ctx->a);
> +       if (err != ARCHIVE_OK && err != ARCHIVE_WARN)
> +               goto error;
> +
> +       if (archive_read_open_filename(ctx->a, name, blocksize) != ARCHIVE_OK)
> +               goto error;
> +
> +       if (archive_read_next_header(ctx->a, &e))

I did a typo when quickly writting the snippet previously.
It should be "archive_read_next_header(ctx->a, &e) != ARCHIVE_OK"


> +               goto error;
> +
>         return 0;
> +
> +error:
> +       archive_read_free(ctx->a);
> +       ctx->a = NULL;
> +       return -1;
>  }
>
>  static ssize_t
> --
> 2.41.0
>
Srikanth Yalavarthi Sept. 26, 2023, 2:47 p.m. UTC | #2 
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: 26 September 2023 19:27
> To: Srikanth Yalavarthi <syalavarthi@marvell.com>
> Cc: Aaron Conole <aconole@redhat.com>; Igor Russkikh
> <irusskikh@marvell.com>; dev@dpdk.org; Shivah Shankar Shankar Narayan
> Rao <sshankarnara@marvell.com>; Anup Prabhu <aprabhu@marvell.com>;
> Prince Takkar <ptakkar@marvell.com>; jerinjacobk@gmail.com;
> stable@dpdk.org
> Subject: [EXT] Re: [PATCH v2 1/1] eal: update xz read support and ignore
> warning
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Tue, Sep 26, 2023 at 3:30 PM Srikanth Yalavarthi
> <syalavarthi@marvell.com> wrote:
> >
> > archive_read_support_filter_xz returns a warning when compression is
> > not fully supported and is supported through external program. This
> > warning can be ignored when reading the files through firmware open as
> > only decompression is required.
> 
> Same comment as before, this sentence is confusing.
> Compressing files does not matter in DPDK rte_firmware_ API.
> 
> The commit title and commitlog won't help people understand in which case
> the issue is reproduced, and how this patch fixes it.
> 
> Suggesting the following title and commitlog:
> 
> """
> eal/unix: fix firmware reading with external xz support
> 
> libarchive may support xz decompression only through an external
> (slower) helper.
> In such a case, archive_read_support_filter_xz() returns ARCHIVE_WARN.
> 
> Fixes: 40edb9c0d36b ("eal: handle compressed firmware")
> Cc: stable@dpdk.org
> """
> 
> >
> > Fixes: 40edb9c0d36b ("eal: handle compressed firmware")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> > Change-Id: I38cce556ec637af03dbde74d7d18318af48082d6
> 
> This is internal scm stuff and must be dropped.
> 
> > ---
> >  lib/eal/unix/eal_firmware.c | 28 ++++++++++++++++++++--------
> >  1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/eal/unix/eal_firmware.c b/lib/eal/unix/eal_firmware.c
> > index d1616b0bd9..16690b4245 100644
> > --- a/lib/eal/unix/eal_firmware.c
> > +++ b/lib/eal/unix/eal_firmware.c
> > @@ -25,19 +25,31 @@ static int
> >  firmware_open(struct firmware_read_ctx *ctx, const char *name, size_t
> > blocksize)  {
> >         struct archive_entry *e;
> > +       int err;
> >
> >         ctx->a = archive_read_new();
> >         if (ctx->a == NULL)
> >                 return -1;
> > -       if (archive_read_support_format_raw(ctx->a) != ARCHIVE_OK ||
> > -                       archive_read_support_filter_xz(ctx->a) != ARCHIVE_OK ||
> > -                       archive_read_open_filename(ctx->a, name, blocksize) !=
> ARCHIVE_OK ||
> > -                       archive_read_next_header(ctx->a, &e) != ARCHIVE_OK) {
> > -               archive_read_free(ctx->a);
> > -               ctx->a = NULL;
> > -               return -1;
> > -       }
> > +
> > +       if (archive_read_support_format_raw(ctx->a) != ARCHIVE_OK)
> > +               goto error;
> > +
> > +       err = archive_read_support_filter_xz(ctx->a);
> > +       if (err != ARCHIVE_OK && err != ARCHIVE_WARN)
> > +               goto error;
> > +
> > +       if (archive_read_open_filename(ctx->a, name, blocksize) !=
> ARCHIVE_OK)
> > +               goto error;
> > +
> > +       if (archive_read_next_header(ctx->a, &e))
> 
> I did a typo when quickly writting the snippet previously.
> It should be "archive_read_next_header(ctx->a, &e) != ARCHIVE_OK"
> 
> 
> > +               goto error;
> > +
> >         return 0;
> > +
> > +error:
> > +       archive_read_free(ctx->a);
> > +       ctx->a = NULL;
> > +       return -1;
> >  }
> >
> >  static ssize_t
> > --
> > 2.41.0
> >
> 

Submitted patch version 4 with suggested changes.

> 
> --
> David Marchand
diff mbox series

Patch

diff --git a/lib/eal/unix/eal_firmware.c b/lib/eal/unix/eal_firmware.c
index d1616b0bd9..16690b4245 100644
--- a/lib/eal/unix/eal_firmware.c
+++ b/lib/eal/unix/eal_firmware.c
@@ -25,19 +25,31 @@  static int
 firmware_open(struct firmware_read_ctx *ctx, const char *name, size_t blocksize)
 {
 	struct archive_entry *e;
+	int err;
 
 	ctx->a = archive_read_new();
 	if (ctx->a == NULL)
 		return -1;
-	if (archive_read_support_format_raw(ctx->a) != ARCHIVE_OK ||
-			archive_read_support_filter_xz(ctx->a) != ARCHIVE_OK ||
-			archive_read_open_filename(ctx->a, name, blocksize) != ARCHIVE_OK ||
-			archive_read_next_header(ctx->a, &e) != ARCHIVE_OK) {
-		archive_read_free(ctx->a);
-		ctx->a = NULL;
-		return -1;
-	}
+
+	if (archive_read_support_format_raw(ctx->a) != ARCHIVE_OK)
+		goto error;
+
+	err = archive_read_support_filter_xz(ctx->a);
+	if (err != ARCHIVE_OK && err != ARCHIVE_WARN)
+		goto error;
+
+	if (archive_read_open_filename(ctx->a, name, blocksize) != ARCHIVE_OK)
+		goto error;
+
+	if (archive_read_next_header(ctx->a, &e))
+		goto error;
+
 	return 0;
+
+error:
+	archive_read_free(ctx->a);
+	ctx->a = NULL;
+	return -1;
 }
 
 static ssize_t