Re: [dpdk-dev] [PATCH 2/2] eal: handle compressed firmwares

[David Marchand <david.marchand@redhat.com>]

On Wed, Jun 2, 2021 at 11:19 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> 2021-06-02 11:58 (UTC+0200), David Marchand:
> > Introduce an internal firmware loading helper to remove code duplication
> > in our drivers and handle xz compressed firmwares by calling libarchive.
> >
> > This helper tries to look for .xz suffixes so that drivers are not aware
> > the firmwares have been compressed.
> >
> > libarchive is set as an optional dependency: without libarchive, a
> > runtime warning is emitted so that users know there is a compressed
> > firmware.
> >
> > Windows implementation is left as an empty stub.
>
> JFYI, it doesn't seem hard to implement as a follow-up if need be.
> libarchive is available for Windows and even provides a .pc file.

I noticed.
But I am not sure the current API is abstracted enough since we pass a
firmware filename.

>
> [...]
> > diff --git a/config/meson.build b/config/meson.build
> > index 017bb2efbb..337daa2719 100644
> > --- a/config/meson.build
> > +++ b/config/meson.build
> > @@ -166,6 +166,15 @@ if fdt_dep.found() and cc.has_header('fdt.h')
> >      dpdk_extra_ldflags += '-lfdt'
> >  endif
> >
> > +has_libarchive = 0
> > +archive_dep = cc.find_library('libarchive', required: false)
> > +if archive_dep.found() and cc.has_header('archive.h')
> > +    dpdk_conf.set10('RTE_HAS_LIBARCHIVE', true)
> > +    has_libarchive = 1
> > +    add_project_link_arguments('-larchive', language: 'c')
> > +    dpdk_extra_ldflags += '-larchive'
> > +endif
> > +
>
> Why not use pkg-config?
> `has_libarchive` is unused.

Frankly, I just copied the logic already present in config/meson.build.
If there is better to do, please advise.


>
> >  libexecinfo = cc.find_library('libexecinfo', required: false)
> >  if libexecinfo.found() and cc.has_header('execinfo.h')
> >      add_project_link_arguments('-lexecinfo', language: 'c')
> [...]
> > diff --git a/lib/eal/version.map b/lib/eal/version.map
> > index fe5c3dac98..020c058e5f 100644
> > --- a/lib/eal/version.map
> > +++ b/lib/eal/version.map
> > @@ -423,11 +423,13 @@ EXPERIMENTAL {
> >       rte_version_release; # WINDOWS_NO_EXPORT
> >       rte_version_suffix; # WINDOWS_NO_EXPORT
> >       rte_version_year; # WINDOWS_NO_EXPORT
> > +
> >  };
>
> Unnecessary empty line.

Yep, already fixed in my local branch.
Thanks.


-- 
David Marchand