3:38 p.m.
On 2/28/19 11:21 AM, Laszlo Ersek wrote:
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(a)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.)
VIR_AUTOFREE() is just a wrapper over __attribute__((cleanup(free))
which is a way to automagically free the memory once corresponding
variable goes out of scope (well, attribute cleanup is more powerful
than that as it can call just any function, but we use it only for
freeing memory in libvirt).
> +
> + 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".
Yeah, we could have something like that. But (my favourite excuse) it is
orthogonal to this patch ;-) There is a lot of code that would need
fixing after virStringList typedef is introduced. I mean fixing as in
replacing char ** with virStringList.
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).
These two are equivallent from virStringList*() APIs POV. All of our
virStringList*() APIs check if passed string list is NULL as the very
first step they do.
Not saying this is wrong, just hard to understand, without docs / typedef.
Yep, I understand that.
> +{
> + 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.
Okay.
> +
> + 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.
Ah, right. So on 32bits ssize_t can actually be smaller than off_t,
since ssize_t is guaranteed to be big enough to address individual bytes
in memory (4GiB in this case) and off_t can address just any byt in a
file (which can be larger than 4GiB so sizeof(off_t) > sizeof(ssize_t)).
Indeed, on my RPi if I print sizeof() with -D_FILE_OFFSET_BITS=64 I get:
ssize_t 4 signed
off_t 8 signed
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.
Ah, didn't know that. Thank you for pointing this out.
> +
> + 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,
Michal