[libvirt] [PATCH v1 09/15] qemu_firmware: Introduce qemuFirmwareFetchConfigs

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> Implementation for yet another part of firmware description
> specification. This one covers selecting which files to parse.
> 
> There are three locations from which description files can be
> loaded. In order of preference, from most generic to most
> specific these are:
> 
>   /usr/share/qemu/firmware
>   /etc/qemu/firmware
>   $XDG_CONFIG_HOME/qemu/firmware
> 
> If a file is find in two or more locations then the most specific

s/find/found/

> one is used. Moreover, if file is empty then it means it is
> overriding some generic description and disabling it.
> 
> Again, this is described in more details and with nice examples
> in firmware.json specification.

Please add a QEMU commit reference here as well.

> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_firmware.c | 127 +++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_firmware.h |   3 +
>  2 files changed, 130 insertions(+)
> 
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index e709a5f608..509927b154 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -21,9 +21,11 @@
>  #include <config.h>
>  
>  #include "qemu_firmware.h"
> +#include "configmake.h"
>  #include "qemu_capabilities.h"
>  #include "virarch.h"
>  #include "virfile.h"
> +#include "virhash.h"
>  #include "virjson.h"
>  #include "virlog.h"
>  #include "virstring.h"
> @@ -899,3 +901,128 @@ qemuFirmwareFormat(qemuFirmwarePtr fw)
>  
>      return virJSONValueToString(doc, true);
>  }
> +
> +
> +static int
> +qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir)
> +{
> +    DIR *dirp;
> +    struct dirent *ent = NULL;
> +    int rc;
> +    int ret = -1;
> +
> +    if ((rc = virDirOpenIfExists(&dirp, dir)) < 0)
> +        return -1;
> +
> +    if (rc == 0)
> +        return 0;
> +
> +    while ((rc = virDirRead(dirp, &ent, dir)) > 0) {
> +        VIR_AUTOFREE(char *) filename = NULL;
> +        VIR_AUTOFREE(char *) path = NULL;

(VIR_AUTOFREE() is very unusual to me, so I could be missing lifecycle
bugs.)

> +
> +        if (ent->d_type != DT_REG && ent->d_type != DT_LNK)
> +            continue;
> +
> +        if (STRPREFIX(ent->d_name, "."))
> +            continue;
> +
> +        if (VIR_STRDUP(filename, ent->d_name) < 0)
> +            goto cleanup;
> +
> +        if (virAsprintf(&path, "%s/%s", dir, filename) < 0)
> +            goto cleanup;
> +
> +        if (virHashUpdateEntry(files, filename, path) < 0)
> +            goto cleanup;
> +
> +        path = NULL;
> +    }
> +
> +    if (rc < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    virDirClose(&dirp);
> +    return ret;
> +}
> +
> +static int
> +qemuFirmwareFilesSorter(const virHashKeyValuePair *a,
> +                        const virHashKeyValuePair *b)
> +{
> +    return strcmp(a->key, b->key);
> +}
> +
> +#define QEMU_FIRMWARE_SYSTEM_LOCATION PREFIX "/share/qemu/firmware"
> +#define QEMU_FIRMWARE_ETC_LOCATION SYSCONFDIR "/qemu/firmware"

You could factor out "qemu/firmware" to a separate macro; you use it
three times in total.

> +
> +int
> +qemuFirmwareFetchConfigs(char ***firmwares)

Apparently, (char **) is a well known type in libvirt (a
"virStringList"). Shouldn't we have an actual typedef for that? It is
surprising not to have one, given the other verbose typedefs for
example, which basically just say "this is a pointer".

I'm also missing some invariants on this type. For example, an empty
string list could be represented by a 1-element array that is
immediately terminated with a NULL. But on failure this function doesn't
produce that, instead it sets *firmwares to NULL (no array at all).

Not saying this is wrong, just hard to understand, without docs / typedef.

> +{
> +    VIR_AUTOPTR(virHashTable) files = NULL;
> +    VIR_AUTOFREE(char *) homeConfig = NULL;
> +    VIR_AUTOFREE(char *) xdgConfig = NULL;
> +    VIR_AUTOFREE(virHashKeyValuePairPtr) pairs = NULL;
> +    virHashKeyValuePairPtr tmp = NULL;
> +
> +    *firmwares = NULL;
> +
> +    if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0)
> +        return -1;
> +
> +    if (!xdgConfig) {
> +        VIR_AUTOFREE(char *) home = virGetUserDirectory();
> +
> +        if (!home)
> +            return -1;
> +
> +        if (virAsprintf(&xdgConfig, "%s/.config", home) < 0)
> +            return -1;
> +    }
> +
> +    if (virAsprintf(&homeConfig, "%s/qemu/firmware", xdgConfig) < 0)
> +        return -1;
> +
> +    if (!(files = virHashCreate(10, virHashValueFree)))
> +        return -1;
> +
> +    if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_SYSTEM_LOCATION) < 0)
> +        return -1;
> +
> +    if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_ETC_LOCATION) < 0)
> +        return -1;
> +
> +    if (qemuFirmwareBuildFileList(files, homeConfig) < 0)
> +        return -1;

OK, so at this point we have a unique set of filenames (one component
only) where each filename (as key) has the highest priority full
pathname associated with it. I'd add a comment about this.

> +
> +    if (virHashSize(files) == 0)
> +        return 0;
> +
> +    if (!(pairs = virHashGetItems(files, qemuFirmwareFilesSorter)))
> +        return -1;
> +
> +    for (tmp = pairs; tmp->key; tmp++) {
> +        const char *path = tmp->value;
> +        off_t len;
> +
> +        if ((len = virFileLength(path, -1)) < 0) {
> +            virReportSystemError(errno,
> +                                 _("unable to get size of %s"),
> +                                 path);
> +            return -1;
> +        }
> +
> +        VIR_DEBUG("firmware description path '%s' len=%zd",
> +                  path, (ssize_t) len);

Sorry about the bike-shedding:

- I suggest sticking with one of "'%s'" and "%s", between the error
report and the debug message (i.e. be consistent in the use of the
apostrophes)

- generally speaking, off_t is not safe to cast to ssize_t; e.g. in an
ILP32_OFFBIG environment, ssize_t could be 32-bit. The only portable way
to print off_t (to my knowledge) is to cast it to intmax_t, and print it
with %jd.

You can see an example for this in the
storageBackendVolZeroSparseFileLocal() function. It seems to expect that
the off_t value in question is nonnegative (which is an expectation that
I cannot comment upon), but then it correctly casts the value to
uintmax_t and prints it with %ju.

> +
> +        if (len == 0)
> +            continue;

Maybe a comment should be added here that the zero size file was
(presumably) used to mask the less specific instances of the same file.

> +
> +        if (virStringListAdd(firmwares, path) < 0)
> +            return -1;
> +    }
> +
> +    return 0;
> +}
> diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h
> index 952615d42b..321169f56c 100644
> --- a/src/qemu/qemu_firmware.h
> +++ b/src/qemu/qemu_firmware.h
> @@ -37,4 +37,7 @@ qemuFirmwareParse(const char *path);
>  char *
>  qemuFirmwareFormat(qemuFirmwarePtr fw);
>  
> +int
> +qemuFirmwareFetchConfigs(char ***firmwares);
> +
>  #endif /* LIBVIRT_QEMU_FIRMWARE_H */
> 

Looks good to me otherwise.

Thanks
Laszlo