The virQEMUDriverConfig object contains lists of loader:nvram pairs to advertise firmwares supported by by the driver, and qemu_conf.c contains code to populate the lists, all of which is useful for other drivers too. To avoid code duplication, introduce a virFirmware object to encapsulate firmware details and switch the qemu driver to use it. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 6 ++ src/qemu/qemu_capabilities.c | 22 +++---- src/qemu/qemu_capabilities.h | 5 +- src/qemu/qemu_conf.c | 127 ++++++---------------------------------- src/qemu/qemu_conf.h | 7 +-- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 6 +- src/util/virfirmware.c | 135 +++++++++++++++++++++++++++++++++++++++++++ src/util/virfirmware.h | 51 ++++++++++++++++ tests/domaincapstest.c | 3 +- 12 files changed, 235 insertions(+), 131 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 0d92448..357d889 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -194,6 +194,7 @@ src/util/virerror.h src/util/vireventpoll.c src/util/virfile.c src/util/virfirewall.c +src/util/virfirmware.c src/util/virhash.c src/util/virhook.c src/util/virhostdev.c diff --git a/src/Makefile.am b/src/Makefile.am index 314f6df..34406cd 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -114,6 +114,7 @@ UTIL_SOURCES = \ util/virfile.c util/virfile.h \ util/virfirewall.c util/virfirewall.h \ util/virfirewallpriv.h \ + util/virfirmware.c util/virfirmware.h \ util/virgettext.c util/virgettext.h \ util/virgic.c util/virgic.h \ util/virhash.c util/virhash.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8408081..e9f117d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1554,6 +1554,12 @@ virFirewallStartRollback; virFirewallStartTransaction; +# util/virfirmware.h +virFirmwareFreeList; +virFirmwareParse; +virFirmwareParseList; + + # util/virgettext.h virGettextInitialize; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 75401cc..e35739b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4060,18 +4060,18 @@ virQEMUCapsGetDefaultMachine(virQEMUCapsPtr qemuCaps) static int virQEMUCapsFillDomainLoaderCaps(virQEMUCapsPtr qemuCaps, virDomainCapsLoaderPtr capsLoader, - char **loader, - size_t nloader) + virFirmwarePtr *firmwares, + size_t nfirmwares) { size_t i; capsLoader->supported = true; - if (VIR_ALLOC_N(capsLoader->values.values, nloader) < 0) + if (VIR_ALLOC_N(capsLoader->values.values, nfirmwares) < 0) return -1; - for (i = 0; i < nloader; i++) { - const char *filename = loader[i]; + for (i = 0; i < nfirmwares; i++) { + const char *filename = firmwares[i]->name; if (!virFileExists(filename)) { VIR_DEBUG("loader filename=%s does not exist", filename); @@ -4102,14 +4102,14 @@ virQEMUCapsFillDomainLoaderCaps(virQEMUCapsPtr qemuCaps, static int virQEMUCapsFillDomainOSCaps(virQEMUCapsPtr qemuCaps, virDomainCapsOSPtr os, - char **loader, - size_t nloader) + virFirmwarePtr *firmwares, + size_t nfirmwares) { virDomainCapsLoaderPtr capsLoader = &os->loader; os->supported = true; if (virQEMUCapsFillDomainLoaderCaps(qemuCaps, capsLoader, - loader, nloader) < 0) + firmwares, nfirmwares) < 0) return -1; return 0; } @@ -4290,8 +4290,8 @@ virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr qemuCaps, int virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, virQEMUCapsPtr qemuCaps, - char **loader, - size_t nloader) + virFirmwarePtr *firmwares, + size_t nfirmwares) { virDomainCapsOSPtr os = &domCaps->os; virDomainCapsDeviceDiskPtr disk = &domCaps->disk; @@ -4303,7 +4303,7 @@ virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, domCaps->maxvcpus = maxvcpus; if (virQEMUCapsFillDomainOSCaps(qemuCaps, os, - loader, nloader) < 0 || + firmwares, nfirmwares) < 0 || virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps, domCaps->machine, disk) < 0 || virQEMUCapsFillDomainDeviceGraphicsCaps(qemuCaps, graphics) < 0 || diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b94a246..386ddc6 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -29,6 +29,7 @@ # include "vircommand.h" # include "qemu_monitor.h" # include "domain_capabilities.h" +# include "virfirmware.h" /* * Internal flags to keep track of qemu command line capabilities @@ -477,7 +478,7 @@ int virQEMUCapsInitGuestFromBinary(virCapsPtr caps, int virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, virQEMUCapsPtr qemuCaps, - char **loader, - size_t nloader); + virFirmwarePtr *firmwares, + size_t nfirmwares); #endif /* __QEMU_CAPABILITIES_H__*/ diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e00ddca..078a37f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -124,47 +124,6 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) } -static int ATTRIBUTE_UNUSED -virQEMUDriverConfigLoaderNVRAMParse(virQEMUDriverConfigPtr cfg, - const char *list) -{ - int ret = -1; - char **token; - size_t i, j; - - if (!(token = virStringSplit(list, ":", 0))) - goto cleanup; - - for (i = 0; token[i]; i += 2) { - if (!token[i] || !token[i + 1] || - STREQ(token[i], "") || STREQ(token[i + 1], "")) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid --with-loader-nvram list: %s"), - list); - goto cleanup; - } - } - - if (i) { - if (VIR_ALLOC_N(cfg->loader, i / 2) < 0 || - VIR_ALLOC_N(cfg->nvram, i / 2) < 0) - goto cleanup; - cfg->nloader = i / 2; - - for (j = 0; j < i / 2; j++) { - if (VIR_STRDUP(cfg->loader[j], token[2 * j]) < 0 || - VIR_STRDUP(cfg->nvram[j], token[2 * j + 1]) < 0) - goto cleanup; - } - } - - ret = 0; - cleanup: - virStringFreeList(token); - return ret; -} - - #define VIR_QEMU_OVMF_LOADER_PATH "/usr/share/OVMF/OVMF_CODE.fd" #define VIR_QEMU_OVMF_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd" #define VIR_QEMU_AAVMF_LOADER_PATH "/usr/share/AAVMF/AAVMF_CODE.fd" @@ -327,20 +286,22 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stdioLogD = true; #ifdef DEFAULT_LOADER_NVRAM - if (virQEMUDriverConfigLoaderNVRAMParse(cfg, DEFAULT_LOADER_NVRAM) < 0) + if (virFirmwareParseList(DEFAULT_LOADER_NVRAM, + &cfg->firmwares, + &cfg->nfirmwares) < 0) goto error; #else - - if (VIR_ALLOC_N(cfg->loader, 2) < 0 || - VIR_ALLOC_N(cfg->nvram, 2) < 0) + if (VIR_ALLOC_N(cfg->firmwares, 2) < 0) + goto error; + cfg->nfirmwares = 2; + if (VIR_ALLOC(cfg->firmwares[0]) < 0 || VIR_ALLOC(cfg->firmwares[1]) < 0) goto error; - cfg->nloader = 2; - if (VIR_STRDUP(cfg->loader[0], VIR_QEMU_AAVMF_LOADER_PATH) < 0 || - VIR_STRDUP(cfg->nvram[0], VIR_QEMU_AAVMF_NVRAM_PATH) < 0 || - VIR_STRDUP(cfg->loader[1], VIR_QEMU_OVMF_LOADER_PATH) < 0 || - VIR_STRDUP(cfg->nvram[1], VIR_QEMU_OVMF_NVRAM_PATH) < 0) + if (VIR_STRDUP(cfg->firwares[0]->name, VIR_QEMU_AAVMF_LOADER_PATH) < 0 || + VIR_STRDUP(cfg->firmwares[0]->nvram, VIR_QEMU_AAVMF_NVRAM_PATH) < 0 || + VIR_STRDUP(cfg->firwares[1]->name, VIR_QEMU_OVMF_LOADER_PATH) < 0 || + VIR_STRDUP(cfg->firmwares[1]->nvram, VIR_QEMU_OVMF_NVRAM_PATH) < 0) goto error; #endif @@ -397,13 +358,7 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->lockManagerName); - while (cfg->nloader) { - VIR_FREE(cfg->loader[cfg->nloader - 1]); - VIR_FREE(cfg->nvram[cfg->nloader - 1]); - cfg->nloader--; - } - VIR_FREE(cfg->loader); - VIR_FREE(cfg->nvram); + virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares); } @@ -427,43 +382,6 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } -static int -virQEMUDriverConfigNVRAMParse(const char *str, - char **loader, - char **nvram) -{ - int ret = -1; - char **token; - - if (!(token = virStringSplit(str, ":", 0))) - goto cleanup; - - if (token[0]) { - virSkipSpaces((const char **) &token[0]); - if (token[1]) - virSkipSpaces((const char **) &token[1]); - } - - /* Exactly two tokens are expected */ - if (!token[0] || !token[1] || token[2] || - STREQ(token[0], "") || STREQ(token[1], "")) { - virReportError(VIR_ERR_CONF_SYNTAX, - _("Invalid nvram format: '%s'"), - str); - goto cleanup; - } - - if (VIR_STRDUP(*loader, token[0]) < 0 || - VIR_STRDUP(*nvram, token[1]) < 0) - goto cleanup; - - ret = 0; - cleanup: - virStringFreeList(token); - return ret; -} - - int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, const char *filename) { @@ -854,14 +772,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, CHECK_TYPE("nvram", VIR_CONF_LIST); - while (cfg->nloader) { - VIR_FREE(cfg->loader[cfg->nloader - 1]); - VIR_FREE(cfg->nvram[cfg->nloader - 1]); - cfg->nloader--; - } - VIR_FREE(cfg->loader); - VIR_FREE(cfg->nvram); - + virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares); /* Calc length and check items */ for (len = 0, pp = p->list; pp; len++, pp = pp->next) { if (pp->type != VIR_CONF_STRING) { @@ -871,16 +782,14 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, } } - if (len && - (VIR_ALLOC_N(cfg->loader, len) < 0 || - VIR_ALLOC_N(cfg->nvram, len) < 0)) + if (len && VIR_ALLOC_N(cfg->firmwares, len) < 0) goto cleanup; - cfg->nloader = len; + cfg->nfirmwares = len; for (i = 0, pp = p->list; pp; i++, pp = pp->next) { - if (virQEMUDriverConfigNVRAMParse(pp->str, - &cfg->loader[i], - &cfg->nvram[i]) < 0) + if (VIR_ALLOC(cfg->firmwares[i]) < 0) + goto cleanup; + if (virFirmwareParse(pp->str, cfg->firmwares[i]) < 0) goto cleanup; } } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index a714b84..30956ff 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -48,6 +48,7 @@ # include "virclosecallbacks.h" # include "virhostdev.h" # include "virfile.h" +# include "virfirmware.h" # ifdef CPU_SETSIZE /* Linux */ # define QEMUD_CPUMASK_LEN CPU_SETSIZE @@ -177,10 +178,8 @@ struct _virQEMUDriverConfig { bool logTimestamp; bool stdioLogD; - /* Pairs of loader:nvram paths. The list is @nloader items long */ - char **loader; - char **nvram; - size_t nloader; + virFirmwarePtr *firmwares; + size_t nfirmwares; }; /* Main driver state */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 37d970e..96a9a3c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18985,7 +18985,7 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, goto cleanup; if (virQEMUCapsFillDomainCaps(domCaps, qemuCaps, - cfg->loader, cfg->nloader) < 0) + cfg->firmwares, cfg->nfirmwares) < 0) goto cleanup; ret = virDomainCapsFormat(domCaps); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f4bf6c1..2749d86 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4121,9 +4121,9 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg, master_nvram_path = loader->templt; if (!loader->templt) { size_t i; - for (i = 0; i < cfg->nloader; i++) { - if (STREQ(cfg->loader[i], loader->path)) { - master_nvram_path = cfg->nvram[i]; + for (i = 0; i < cfg->nfirmwares; i++) { + if (STREQ(cfg->firmwares[i]->name, loader->path)) { + master_nvram_path = cfg->firmwares[i]->nvram; break; } } diff --git a/src/util/virfirmware.c b/src/util/virfirmware.c new file mode 100644 index 0000000..e0eb6b9 --- /dev/null +++ b/src/util/virfirmware.c @@ -0,0 +1,135 @@ +/* + * virfirmware.c: Definition of firmware object and supporting functions + * + * Copyright (C) 2016 SUSE LINUX Products GmbH, Nuernberg, Germany. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + * + * Author: Jim Fehlig <jfehlig(a)suse.com&gt; + */ + +#include <config.h> + +#include "viralloc.h" +#include "virerror.h" +#include "virfirmware.h" +#include "virlog.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.firmware"); + + +static void +virFirmwareFree(virFirmwarePtr firmware) +{ + if (!firmware) + return; + + VIR_FREE(firmware->name); + VIR_FREE(firmware->nvram); + VIR_FREE(firmware); +} + + +void +virFirmwareFreeList(virFirmwarePtr *firmwares, size_t nfirmwares) +{ + size_t i; + + for (i = 0; i < nfirmwares; i++) + virFirmwareFree(firmwares[i]); +} + + +int +virFirmwareParse(const char *str, virFirmwarePtr firmware) +{ + int ret = -1; + char **token; + + if (!(token = virStringSplit(str, ":", 0))) + goto cleanup; + + if (token[0]) { + virSkipSpaces((const char **) &token[0]); + if (token[1]) + virSkipSpaces((const char **) &token[1]); + } + + /* Exactly two tokens are expected */ + if (!token[0] || !token[1] || token[2] || + STREQ(token[0], "") || STREQ(token[1], "")) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("Invalid nvram format: '%s'"), + str); + goto cleanup; + } + + if (VIR_STRDUP(firmware->name, token[0]) < 0 || + VIR_STRDUP(firmware->nvram, token[1]) < 0) + goto cleanup; + + ret = 0; + cleanup: + virStringFreeList(token); + return ret; +} + + +int +virFirmwareParseList(const char *list, + virFirmwarePtr **firmwares, + size_t *nfirmwares) +{ + int ret = -1; + char **token; + size_t i, j; + + if (!(token = virStringSplit(list, ":", 0))) + goto cleanup; + + for (i = 0; token[i]; i += 2) { + if (!token[i] || !token[i + 1] || + STREQ(token[i], "") || STREQ(token[i + 1], "")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid --with-loader-nvram list: %s"), + list); + goto cleanup; + } + } + + if (i) { + if (VIR_ALLOC_N(*firmwares, i / 2) < 0) + goto cleanup; + *nfirmwares = i / 2; + + for (j = 0; j < i / 2; j++) { + virFirmwarePtr *fws = *firmwares; + + if (VIR_ALLOC(fws[j]) < 0) + goto cleanup; + if (VIR_STRDUP(fws[j]->name, token[2 * j]) < 0 || + VIR_STRDUP(fws[j]->nvram, token[2 * j + 1]) < 0) + goto cleanup; + } + } + + ret = 0; + cleanup: + virStringFreeList(token); + return ret; +} diff --git a/src/util/virfirmware.h b/src/util/virfirmware.h new file mode 100644 index 0000000..682a865 --- /dev/null +++ b/src/util/virfirmware.h @@ -0,0 +1,51 @@ +/* + * virfirmware.h: Declaration of firmware object and supporting functions + * + * Copyright (C) 2016 SUSE LINUX Products GmbH, Nuernberg, Germany. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + * + * Author: Jim Fehlig <jfehlig(a)suse.com&gt; + */ + +#ifndef __VIR_FIRMWARE_H__ +# define __VIR_FIRMWARE_H__ + +# include "internal.h" + +typedef struct _virFirmware virFirmware; +typedef virFirmware *virFirmwarePtr; + +struct _virFirmware { + char *name; + char *nvram; +}; + + +void +virFirmwareFreeList(virFirmwarePtr *firmwares, size_t nfirmwares); + +int +virFirmwareParse(const char *str, virFirmwarePtr firmware) + ATTRIBUTE_NONNULL(2); + +int +virFirmwareParseList(const char *list, + virFirmwarePtr **firmwares, + size_t *nfirmwares) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + + +#endif /* __VIR_FIRMWARE_H__ */ diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index 3436197..e74b34a 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -128,7 +128,8 @@ fillQemuCaps(virDomainCapsPtr domCaps, goto cleanup; if (virQEMUCapsFillDomainCaps(domCaps, qemuCaps, - cfg->loader, cfg->nloader) < 0) + cfg->firmwares, + cfg->nfirmwares) < 0) goto cleanup; /* The function above tries to query host's KVM & VFIO capabilities by -- 2.1.4

On 18.05.2016 18:38, Jim Fehlig wrote: s/firware/firmware/g As you point out too, @firmwares needs to be freed here. ACK with those nits fixed. Michal

On 18.05.2016 18:38, Jim Fehlig wrote: Yet again, copy & paste error :) s/firwares/firmwares/g Well, there's technically nothing wrong with this patch, but perhaps we want to report the information we parse here? I mean, so far this is just a setter and no getter. Are those patches yet to come? Michal

On 25.05.2016 15:28, Jim Fehlig wrote: Ah, haven't seen those while reviewing this one. ACK to this one then too. Now it makes perfect sense. Michal