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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Wed, Jun 2, 2021 at 3:29 PM David Marchand <david.marchand@redhat.com> wrote:
>
> 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.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

> + */
> +
> +#ifndef __RTE_FIRMWARE_H__
> +#define __RTE_FIRMWARE_H__
> +
> +#include <sys/types.h>
> +
> +#include <rte_compat.h>
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Load a firmware in a dynamically allocated buffer, dealing with compressed
> + * files if libarchive is available.
> + *
> + * @param name
> + *      Firmware filename to load.
> + * @param buf

Adding out to express the output useful. i.e
@param[out] buf

> + *      Buffer allocated by this function. If this function succeeds, the
> + *      caller is responsible for freeing the buffer.

I think, we can chnange to "freeing the buffer using free()" to avoid
confusion with rte_free()

> + * @param bufsz

@param[out] bufsz

> + *      Size of the data in the buffer.
> + *
> + * @return
> + *      0 if successful.
> + *      Negative otherwise, buf and bufsize contents are invalid.
> + */
> +__rte_internal
> +int
> +rte_firmware_read(const char *name, void **buf, size_t *bufsz);
> +
> +#endif
> diff --git a/lib/eal/unix/eal_firmware.c b/lib/eal/unix/eal_firmware.c
> new file mode 100644
> index 0000000000..ea66fecfe9
> --- /dev/null
> +++ b/lib/eal/unix/eal_firmware.c
> @@ -0,0 +1,106 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2021 Red Hat, Inc.
> + */
> +
> +#ifdef RTE_HAS_LIBARCHIVE
> +#include <archive.h>
> +#endif
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +#include <rte_common.h>
> +#include <rte_firmware.h>
> +#include <rte_log.h>
> +
> +static int
> +firmware_read(const char *name, void **buf, size_t *bufsz)
> +{
> +       const size_t blocksize = 4096;
> +       int ret = -1;
> +       int err;
> +#ifdef RTE_HAS_LIBARCHIVE


I think, better to have small inline functions for libarchive variant
vs normal file accessors
in the group for open, access, read etc to avoid the ifdef clutter and
manage with one ifdef.



> +       struct archive_entry *entry;
> +       struct archive *a;
> +#else
> +       int fd;
> +#endif
> +
> +       *buf = NULL;
> +       *bufsz = 0;
> +
> +#ifdef RTE_HAS_LIBARCHIVE

See above

> +       a = archive_read_new();
> +       if (a == NULL || archive_read_support_format_raw(a) != ARCHIVE_OK ||
> +                       archive_read_support_filter_xz(a) != ARCHIVE_OK ||
> +                       archive_read_open_filename(a, name, blocksize) != ARCHIVE_OK ||
> +                       archive_read_next_header(a, &entry) != ARCHIVE_OK)
> +               goto out;
> +#else
> +       fd = open(name, O_RDONLY);
> +       if (fd < 0)
> +               goto out;
> +#endif
> +
> +       do {
> +               void *tmp;
> +
> +               tmp = realloc(*buf, *bufsz + blocksize);
> +               if (tmp == NULL) {
> +                       free(*buf);
> +                       *buf = NULL;
> +                       *bufsz = 0;
> +                       break;
> +               }
> +               *buf = tmp;
> +
> +#ifdef RTE_HAS_LIBARCHIVE

See above

> +               err = archive_read_data(a, RTE_PTR_ADD(*buf, *bufsz), blocksize);
> +#else
> +               err = read(fd, RTE_PTR_ADD(*buf, *bufsz), blocksize);
> +#endif
> +               if (err < 0) {
> +                       free(*buf);
> +                       *buf = NULL;
> +                       *bufsz = 0;
> +                       break;
> +               }
> +               *bufsz += err;
> +
> +       } while (err != 0);
> +
> +       if (*buf != NULL)
> +               ret = 0;
> +out:
> +#ifdef RTE_HAS_LIBARCHIVE

See above

> +       if (a != NULL)
> +               archive_read_free(a);
> +#else
> +       if (fd >= 0)
> +               close(fd);
> +#endif
> +       return ret;
> +}
> +
> +int
> +rte_firmware_read(const char *name, void **buf, size_t *bufsz)
> +{
> +       char path[PATH_MAX];
> +       int ret;
> +
> +       ret = firmware_read(name, buf, bufsz);
> +       if (ret < 0) {
> +               snprintf(path, sizeof(path), "%s.xz", name);
> +               path[PATH_MAX - 1] = '\0';
> +#ifndef RTE_HAS_LIBARCHIVE

See above

> +               if (access(path, F_OK) == 0) {
> +                       RTE_LOG(WARNING, EAL, "libarchive not available, %s cannot be decompressed\n",
> +                               path);
> +               }
> +#else
> +               ret = firmware_read(path, buf, bufsz);
> +#endif
>