[libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
This series is required to solve a known limitation in the current incarnation of device isolation support[1], namely the inability to isolate host devices coming from IOMMU group 0. The issue lies in the fact that virDomainDeviceInfo, and all virDomain*Def that embed it, are usually allocated through VIR_ALLOC(), which result in all their fields being initialized to zero. That's worked out just fine so far, because zero was a sensible default value for all existing fields; however, when implementing isolation groups, we add a new virDomainDeviceInfo::isolationGroup field which we need to be initialized to -1 instead so that it doesn't overlap with IOMMU group 0 mentioned above. Solving the issue involves creating twenty or so virDomain*DefNew() functions and using them instead of VIR_ALLOC() every time a virDomain*Def needs to be created, which is arguably a pretty good idea regardless. The series could probably be organized so that eg. the patch order makes more sense, but honestly I'm pretty tired right now and I just don't have it in me. Moreover, the goal of the series and its implementation are both straighforward enough that I feel reviewers will have no problem following along. My main concern with this series is that, while converting from VIR_ALLOC() to virDomain*DefNew(), I might simply have missed some instances. That would not cause any issue right away, but once we introduce isolation groups it would lead to suboptimal PCI addresses being allocated at best, and to perfectly legitimate hotplug requests being denied at worst. The fact that the test suite[2] passes gives me a lot of confidence that I haven't overlooked anything, but if anyone has clever ideas on how to be actually sure rather than merely confident please do let me know. [1] https://www.redhat.com/archives/libvir-list/2017-June/msg01018.html [2] That is, the version of it where the default isolation group being initialized correctly actually matters Andrea Bolognani (10): conf: Make virDomainDeviceGetInfo() private conf: Rename virDomainHostdevDefAlloc() to virDomainHostdevDefNew() conf: Clean up virDomainHostdevDefNew() conf: Clean up virDomain{Memory,Shmem}DefParseXML() conf: Move some virDomainDeviceInfo functions conf: Introduce virDomainDeviceInfoNew() conf: Clean up virDomainDeviceInfo functions conf, qemu: Use virDomainDeviceInfoNew() conf: Provide missing virDomain*DefNew() functions conf: Call virDomainDeviceInfoClear() in virDomain*DefNew() src/bhyve/bhyve_parse_command.c | 4 +- src/conf/device_conf.c | 146 ++++++++++++ src/conf/device_conf.h | 9 + src/conf/domain_addr.c | 6 +- src/conf/domain_conf.c | 486 ++++++++++++++++++++++++---------------- src/conf/domain_conf.h | 25 ++- src/libvirt_private.syms | 19 +- src/lxc/lxc_native.c | 2 +- src/openvz/openvz_conf.c | 2 +- src/qemu/qemu_command.c | 12 +- src/qemu/qemu_domain.c | 11 +- src/qemu/qemu_domain_address.c | 31 +-- src/qemu/qemu_hotplug.c | 5 +- src/qemu/qemu_parse_command.c | 31 +-- src/vbox/vbox_common.c | 14 +- src/vmx/vmx.c | 2 +- src/vz/vz_sdk.c | 6 +- src/xen/xen_driver.c | 2 +- src/xenapi/xenapi_driver.c | 2 +- src/xenconfig/xen_common.c | 4 +- src/xenconfig/xen_sxpr.c | 10 +- src/xenconfig/xen_xl.c | 4 +- src/xenconfig/xen_xm.c | 2 +- tests/virhostdevtest.c | 2 +- 24 files changed, 561 insertions(+), 276 deletions(-) -- 2.7.5
There are no external users. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 21 ++++++++++++--------- src/conf/domain_conf.h | 1 - 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c3149f9..d9aea83 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1322,6 +1322,18 @@ bool virDomainObjTaint(virDomainObjPtr obj, return true; } + +static void +virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) +{ + VIR_FREE(info->alias); + memset(&info->addr, 0, sizeof(info->addr)); + info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; + VIR_FREE(info->romfile); + VIR_FREE(info->loadparm); +} + + static void virDomainDeviceInfoFree(virDomainDeviceInfoPtr info) { @@ -3591,15 +3603,6 @@ virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, return 0; } -void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) -{ - VIR_FREE(info->alias); - memset(&info->addr, 0, sizeof(info->addr)); - info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; - VIR_FREE(info->romfile); - VIR_FREE(info->loadparm); -} - static bool virDomainSkipBackcompatConsole(virDomainDefPtr def, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 964bc02..9dc64b2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2701,7 +2701,6 @@ int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device); int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, virDomainDeviceInfoPtr src); -void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); void virDomainTPMDefFree(virDomainTPMDefPtr def); typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, -- 2.7.5
On Thu, Jun 29, 2017 at 20:03:54 +0200, Andrea Bolognani wrote:
There are no external users.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 21 ++++++++++++--------- src/conf/domain_conf.h | 1 - 2 files changed, 12 insertions(+), 10 deletions(-)
ACK
All other virDomain*Def follow this naming convention for their allocation function. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 +- src/lxc/lxc_native.c | 2 +- src/qemu/qemu_parse_command.c | 4 ++-- src/vbox/vbox_common.c | 2 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- src/xenconfig/xen_xl.c | 2 +- tests/virhostdevtest.c | 2 +- 10 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d9aea83..15dd285 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2378,7 +2378,7 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def) virDomainHostdevDefPtr -virDomainHostdevDefAlloc(virDomainXMLOptionPtr xmlopt) +virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt) { virDomainHostdevDefPtr def = NULL; @@ -13801,7 +13801,7 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, ctxt->node = node; - if (!(def = virDomainHostdevDefAlloc(xmlopt))) + if (!(def = virDomainHostdevDefNew(xmlopt))) goto error; if (mode) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9dc64b2..cbcffa4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2684,7 +2684,7 @@ void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def); void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def); void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); void virDomainVideoDefFree(virDomainVideoDefPtr def); -virDomainHostdevDefPtr virDomainHostdevDefAlloc(virDomainXMLOptionPtr xmlopt); +virDomainHostdevDefPtr virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt); void virDomainHostdevDefClear(virDomainHostdevDefPtr def); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainHubDefFree(virDomainHubDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 888412a..e92fe52 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -366,9 +366,9 @@ virDomainGraphicsVNCSharePolicyTypeFromString; virDomainGraphicsVNCSharePolicyTypeToString; virDomainHasNet; virDomainHostdevCapsTypeToString; -virDomainHostdevDefAlloc; virDomainHostdevDefClear; virDomainHostdevDefFree; +virDomainHostdevDefNew; virDomainHostdevFind; virDomainHostdevInsert; virDomainHostdevModeTypeToString; diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index 8f44168..7bcdf1f 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -394,7 +394,7 @@ lxcCreateNetDef(const char *type, static virDomainHostdevDefPtr lxcCreateHostdevDef(int mode, int type, const char *data) { - virDomainHostdevDefPtr hostdev = virDomainHostdevDefAlloc(NULL); + virDomainHostdevDefPtr hostdev = virDomainHostdevDefNew(NULL); if (!hostdev) return NULL; diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index af9063c..90e8d09 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -1162,7 +1162,7 @@ qemuParseCommandLinePCI(const char *val) int bus = 0, slot = 0, func = 0; const char *start; char *end; - virDomainHostdevDefPtr def = virDomainHostdevDefAlloc(NULL); + virDomainHostdevDefPtr def = virDomainHostdevDefNew(NULL); if (!def) goto error; @@ -1212,7 +1212,7 @@ qemuParseCommandLinePCI(const char *val) static virDomainHostdevDefPtr qemuParseCommandLineUSB(const char *val) { - virDomainHostdevDefPtr def = virDomainHostdevDefAlloc(NULL); + virDomainHostdevDefPtr def = virDomainHostdevDefNew(NULL); virDomainHostdevSubsysUSBPtr usbsrc; int first = 0, second = 0; const char *start; diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index c46e71b..92ee371 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -2989,7 +2989,7 @@ vboxHostDeviceGetXMLDesc(vboxDriverPtr data, virDomainDefPtr def, IMachine *mach goto release_filters; for (i = 0; i < def->nhostdevs; i++) { - def->hostdevs[i] = virDomainHostdevDefAlloc(NULL); + def->hostdevs[i] = virDomainHostdevDefNew(NULL); if (!def->hostdevs[i]) goto release_hostdevs; } diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 72ffb30..6d7dc2c 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -458,7 +458,7 @@ xenParsePCI(virConfPtr conf, virDomainDefPtr def) goto skippci; if (virStrToLong_i(func, NULL, 16, &funcID) < 0) goto skippci; - if (!(hostdev = virDomainHostdevDefAlloc(NULL))) + if (!(hostdev = virDomainHostdevDefNew(NULL))) return -1; hostdev->managed = false; diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index baa68b1..fefa61a 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -1110,7 +1110,7 @@ xenParseSxprPCI(virDomainDefPtr def, goto error; } - if (!(dev = virDomainHostdevDefAlloc(NULL))) + if (!(dev = virDomainHostdevDefNew(NULL))) goto error; dev->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index cac440c..fa3f1d0 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -733,7 +733,7 @@ xenParseXLUSB(virConfPtr conf, virDomainDefPtr def) goto skipusb; if (virStrToLong_i(device, NULL, 16, &devNum) < 0) goto skipusb; - if (!(hostdev = virDomainHostdevDefAlloc(NULL))) + if (!(hostdev = virDomainHostdevDefNew(NULL))) return -1; hostdev->managed = false; diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index b3beee9..655991c 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -87,7 +87,7 @@ myInit(void) for (i = 0; i < nhostdevs; i++) { virDomainHostdevSubsys subsys; - hostdevs[i] = virDomainHostdevDefAlloc(NULL); + hostdevs[i] = virDomainHostdevDefNew(NULL); if (!hostdevs[i]) goto cleanup; hostdevs[i]->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; -- 2.7.5
On Thu, Jun 29, 2017 at 20:03:55 +0200, Andrea Bolognani wrote:
All other virDomain*Def follow this naming convention for their allocation function.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 +- src/lxc/lxc_native.c | 2 +- src/qemu/qemu_parse_command.c | 4 ++-- src/vbox/vbox_common.c | 2 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- src/xenconfig/xen_xl.c | 2 +- tests/virhostdevtest.c | 2 +- 10 files changed, 12 insertions(+), 12 deletions(-)
ACK
Follow the same style as other similar functions. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 15dd285..ccd3c27 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2380,25 +2380,26 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def) virDomainHostdevDefPtr virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt) { - virDomainHostdevDefPtr def = NULL; + virDomainHostdevDefPtr def; - if (VIR_ALLOC(def) < 0 || - VIR_ALLOC(def->info) < 0) { - VIR_FREE(def); + if (VIR_ALLOC(def) < 0) return NULL; - } + + if (VIR_ALLOC(def->info) < 0) + goto error; if (xmlopt && xmlopt->privateData.hostdevNew && !(def->privateData = xmlopt->privateData.hostdevNew())) goto error; + cleanup: return def; error: VIR_FREE(def->info); VIR_FREE(def); - return NULL; + goto cleanup; } -- 2.7.5
On Thu, Jun 29, 2017 at 20:03:56 +0200, Andrea Bolognani wrote:
Follow the same style as other similar functions.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 15dd285..ccd3c27 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2380,25 +2380,26 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def) virDomainHostdevDefPtr virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt) { - virDomainHostdevDefPtr def = NULL; + virDomainHostdevDefPtr def;
- if (VIR_ALLOC(def) < 0 || - VIR_ALLOC(def->info) < 0) { - VIR_FREE(def); + if (VIR_ALLOC(def) < 0) return NULL; - } + + if (VIR_ALLOC(def->info) < 0) + goto error;
if (xmlopt && xmlopt->privateData.hostdevNew && !(def->privateData = xmlopt->privateData.hostdevNew())) goto error;
Changes below a rather pointless, and nothing gets added to the cleanup section in other patches. ACK to changes above this line.
+ cleanup: return def;
error: VIR_FREE(def->info); VIR_FREE(def); - return NULL; + goto cleanup; }
-- 2.7.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Follow the same style as other similar functions. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ccd3c27..fdb919d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13143,8 +13143,7 @@ virDomainShmemDefParseXML(xmlNodePtr node, unsigned int flags) { char *tmp = NULL; - virDomainShmemDefPtr def = NULL; - virDomainShmemDefPtr ret = NULL; + virDomainShmemDefPtr def; xmlNodePtr msi = NULL; xmlNodePtr save = ctxt->node; xmlNodePtr server = NULL; @@ -13163,7 +13162,7 @@ virDomainShmemDefParseXML(xmlNodePtr node, if ((def->model = virDomainShmemModelTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Unknown shmem model type '%s'"), tmp); - goto cleanup; + goto error; } VIR_FREE(tmp); @@ -13172,12 +13171,12 @@ virDomainShmemDefParseXML(xmlNodePtr node, if (!(def->name = virXMLPropString(node, "name"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("shmem element must contain 'name' attribute")); - goto cleanup; + goto error; } if (virDomainParseScaledValue("./size[1]", NULL, ctxt, &def->size, 1, ULLONG_MAX, false) < 0) - goto cleanup; + goto error; if ((server = virXPathNode("./server[1]", ctxt))) { def->server.enabled = true; @@ -13197,7 +13196,7 @@ virDomainShmemDefParseXML(xmlNodePtr node, virReportError(VIR_ERR_XML_ERROR, _("invalid number of vectors for shmem: '%s'"), tmp); - goto cleanup; + goto error; } VIR_FREE(tmp); @@ -13208,7 +13207,7 @@ virDomainShmemDefParseXML(xmlNodePtr node, virReportError(VIR_ERR_XML_ERROR, _("invalid msi ioeventfd setting for shmem: '%s'"), tmp); - goto cleanup; + goto error; } def->msi.ioeventfd = val; } @@ -13219,20 +13218,21 @@ virDomainShmemDefParseXML(xmlNodePtr node, if (def->msi.enabled && !def->server.enabled) { virReportError(VIR_ERR_XML_ERROR, "%s", _("msi option is only supported with a server")); - goto cleanup; + goto error; } if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) - goto cleanup; - + goto error; - ret = def; - def = NULL; cleanup: - ctxt->node = save; VIR_FREE(tmp); + ctxt->node = save; + return def; + + error: virDomainShmemDefFree(def); - return ret; + def = NULL; + goto cleanup; } static int @@ -14394,7 +14394,6 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode, def->access = val; } - VIR_FREE(tmp); /* source */ if ((node = virXPathNode("./source", ctxt)) && @@ -14414,14 +14413,15 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode, if (virDomainDeviceInfoParseXML(memdevNode, NULL, &def->info, flags) < 0) goto error; + cleanup: + VIR_FREE(tmp); ctxt->node = save; return def; error: - VIR_FREE(tmp); virDomainMemoryDefFree(def); - ctxt->node = save; - return NULL; + def = NULL; + goto cleanup; } -- 2.7.5
On Thu, Jun 29, 2017 at 20:03:57 +0200, Andrea Bolognani wrote:
Follow the same style as other similar functions.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ccd3c27..fdb919d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -13219,20 +13218,21 @@ virDomainShmemDefParseXML(xmlNodePtr node, if (def->msi.enabled && !def->server.enabled) { virReportError(VIR_ERR_XML_ERROR, "%s", _("msi option is only supported with a server")); - goto cleanup; + goto error; }
if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) - goto cleanup; - + goto error;
- ret = def; - def = NULL; cleanup: - ctxt->node = save; VIR_FREE(tmp); + ctxt->node = save; + return def; + + error: virDomainShmemDefFree(def); - return ret; + def = NULL; + goto cleanup;
I don't see how this is better than it was before.
On Fri, 2017-06-30 at 12:50 +0200, Peter Krempa wrote:
if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) - goto cleanup; - + goto error; - ret = def; - def = NULL; cleanup: - ctxt->node = save; VIR_FREE(tmp); + ctxt->node = save; + return def; + + error: virDomainShmemDefFree(def); - return ret; + def = NULL; + goto cleanup; I don't see how this is better than it was before.
It's only better in that it follows the same structure as other *ParseXML() functions in the same file. Consistency FTW! Plus we can drop the 'ret' local variable. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jun 30, 2017 at 13:17:04 +0200, Andrea Bolognani wrote:
On Fri, 2017-06-30 at 12:50 +0200, Peter Krempa wrote:
if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) - goto cleanup; - + goto error; - ret = def; - def = NULL; cleanup: - ctxt->node = save; VIR_FREE(tmp); + ctxt->node = save; + return def; + + error: virDomainShmemDefFree(def); - return ret; + def = NULL; + goto cleanup; I don't see how this is better than it was before.
It's only better in that it follows the same structure as other *ParseXML() functions in the same file. Consistency FTW! Plus we can drop the 'ret' local variable.
Okay, NACK to the change then. I prefer having less labels.
The virDomainDeviceInfo struct is defined in device_conf, so generic functions that operate on it should also be defined there rather than in domain_conf. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/device_conf.c | 109 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/device_conf.h | 8 ++++ src/conf/domain_conf.c | 114 ----------------------------------------------- src/conf/domain_conf.h | 7 --- src/libvirt_private.syms | 4 +- 5 files changed, 119 insertions(+), 123 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index f58b9d0..4644580 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -32,6 +32,115 @@ #define VIR_FROM_THIS VIR_FROM_DEVICE +void +virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) +{ + VIR_FREE(info->alias); + memset(&info->addr, 0, sizeof(info->addr)); + info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; + VIR_FREE(info->romfile); + VIR_FREE(info->loadparm); +} + +int +virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, + virDomainDeviceInfoPtr src) +{ + /* Assume that dst is already cleared */ + + /* first a shallow copy of *everything* */ + *dst = *src; + + /* then redo the two fields that are pointers */ + dst->alias = NULL; + dst->romfile = NULL; + + if (VIR_STRDUP(dst->alias, src->alias) < 0 || + VIR_STRDUP(dst->romfile, src->romfile) < 0) + return -1; + return 0; +} + +void +virDomainDeviceInfoFree(virDomainDeviceInfoPtr info) +{ + if (info) { + virDomainDeviceInfoClear(info); + VIR_FREE(info); + } +} + +bool +virDomainDeviceInfoAddressIsEqual(const virDomainDeviceInfo *a, + const virDomainDeviceInfo *b) +{ + if (a->type != b->type) + return false; + + switch ((virDomainDeviceAddressType) a->type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + /* address types below don't have any specific data */ + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: + /* the 'multi' field shouldn't be checked */ + if (a->addr.pci.domain != b->addr.pci.domain || + a->addr.pci.bus != b->addr.pci.bus || + a->addr.pci.slot != b->addr.pci.slot || + a->addr.pci.function != b->addr.pci.function) + return false; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: + if (memcmp(&a->addr.drive, &b->addr.drive, sizeof(a->addr.drive))) + return false; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL: + if (memcmp(&a->addr.vioserial, &b->addr.vioserial, sizeof(a->addr.vioserial))) + return false; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID: + if (memcmp(&a->addr.ccid, &b->addr.ccid, sizeof(a->addr.ccid))) + return false; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: + if (memcmp(&a->addr.usb, &b->addr.usb, sizeof(a->addr.usb))) + return false; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (memcmp(&a->addr.spaprvio, &b->addr.spaprvio, sizeof(a->addr.spaprvio))) + return false; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: + /* the 'assigned' field denotes that the address was generated */ + if (a->addr.ccw.cssid != b->addr.ccw.cssid || + a->addr.ccw.ssid != b->addr.ccw.ssid || + a->addr.ccw.devno != b->addr.ccw.devno) + return false; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA: + if (memcmp(&a->addr.isa, &b->addr.isa, sizeof(a->addr.isa))) + return false; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM: + if (memcmp(&a->addr.dimm, &b->addr.dimm, sizeof(a->addr.dimm))) + return false; + break; + } + + return true; +} + int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr, bool report) { diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 48782be..53abe1b 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -170,6 +170,14 @@ struct _virDomainDeviceInfo { char *loadparm; }; +void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); +int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, + virDomainDeviceInfoPtr src); +void virDomainDeviceInfoFree(virDomainDeviceInfoPtr info); + +bool virDomainDeviceInfoAddressIsEqual(const virDomainDeviceInfo *a, + const virDomainDeviceInfo *b) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr, bool report); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fdb919d..6a89fab 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1324,27 +1324,6 @@ bool virDomainObjTaint(virDomainObjPtr obj, static void -virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) -{ - VIR_FREE(info->alias); - memset(&info->addr, 0, sizeof(info->addr)); - info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; - VIR_FREE(info->romfile); - VIR_FREE(info->loadparm); -} - - -static void -virDomainDeviceInfoFree(virDomainDeviceInfoPtr info) -{ - if (info) { - virDomainDeviceInfoClear(info); - VIR_FREE(info); - } -} - - -static void virDomainGraphicsAuthDefClear(virDomainGraphicsAuthDefPtr def) { if (!def) @@ -3495,77 +3474,6 @@ virDomainDeviceInfoNeedsFormat(virDomainDeviceInfoPtr info, unsigned int flags) return false; } -bool -virDomainDeviceInfoAddressIsEqual(const virDomainDeviceInfo *a, - const virDomainDeviceInfo *b) -{ - if (a->type != b->type) - return false; - - switch ((virDomainDeviceAddressType) a->type) { - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: - /* address types below don't have any specific data */ - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: - break; - - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - /* the 'multi' field shouldn't be checked */ - if (a->addr.pci.domain != b->addr.pci.domain || - a->addr.pci.bus != b->addr.pci.bus || - a->addr.pci.slot != b->addr.pci.slot || - a->addr.pci.function != b->addr.pci.function) - return false; - break; - - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: - if (memcmp(&a->addr.drive, &b->addr.drive, sizeof(a->addr.drive))) - return false; - break; - - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL: - if (memcmp(&a->addr.vioserial, &b->addr.vioserial, sizeof(a->addr.vioserial))) - return false; - break; - - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID: - if (memcmp(&a->addr.ccid, &b->addr.ccid, sizeof(a->addr.ccid))) - return false; - break; - - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: - if (memcmp(&a->addr.usb, &b->addr.usb, sizeof(a->addr.usb))) - return false; - break; - - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: - if (memcmp(&a->addr.spaprvio, &b->addr.spaprvio, sizeof(a->addr.spaprvio))) - return false; - break; - - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: - /* the 'assigned' field denotes that the address was generated */ - if (a->addr.ccw.cssid != b->addr.ccw.cssid || - a->addr.ccw.ssid != b->addr.ccw.ssid || - a->addr.ccw.devno != b->addr.ccw.devno) - return false; - break; - - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA: - if (memcmp(&a->addr.isa, &b->addr.isa, sizeof(a->addr.isa))) - return false; - break; - - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM: - if (memcmp(&a->addr.dimm, &b->addr.dimm, sizeof(a->addr.dimm))) - return false; - break; - } - - return true; -} - static int virDomainDefHasDeviceAddressIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, @@ -3583,28 +3491,6 @@ virDomainDefHasDeviceAddressIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, } -int -virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, - virDomainDeviceInfoPtr src) -{ - /* Assume that dst is already cleared */ - - /* first a shallow copy of *everything* */ - *dst = *src; - - /* then copy whatever's left */ - dst->alias = NULL; - dst->romfile = NULL; - dst->loadparm = NULL; - - if (VIR_STRDUP(dst->alias, src->alias) < 0 || - VIR_STRDUP(dst->romfile, src->romfile) < 0 || - VIR_STRDUP(dst->loadparm, src->loadparm) < 0) - return -1; - return 0; -} - - static bool virDomainSkipBackcompatConsole(virDomainDefPtr def, size_t idx, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cbcffa4..00d0d65 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2699,8 +2699,6 @@ virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src, int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int type); virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device); -int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, - virDomainDeviceInfoPtr src); void virDomainTPMDefFree(virDomainTPMDefPtr def); typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, @@ -3323,11 +3321,6 @@ virDomainGetBlkioParametersAssignFromDef(virDomainDefPtr def, int *nparams, int maxparams); -bool -virDomainDeviceInfoAddressIsEqual(const virDomainDeviceInfo *a, - const virDomainDeviceInfo *b) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; - int virDomainDiskSetBlockIOTune(virDomainDiskDefPtr disk, virDomainBlockIoTuneInfo *info); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e92fe52..178aefc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -89,6 +89,8 @@ virCPUModeTypeToString; # conf/device_conf.h +virDomainDeviceInfoAddressIsEqual; +virDomainDeviceInfoCopy; virInterfaceLinkFormat; virInterfaceLinkParseXML; virPCIDeviceAddressEqual; @@ -285,8 +287,6 @@ virDomainDeviceDefFree; virDomainDeviceDefParse; virDomainDeviceFindControllerModel; virDomainDeviceGetInfo; -virDomainDeviceInfoAddressIsEqual; -virDomainDeviceInfoCopy; virDomainDeviceInfoIterate; virDomainDeviceTypeToString; virDomainDiskBusTypeToString; -- 2.7.5
On Thu, Jun 29, 2017 at 20:03:58 +0200, Andrea Bolognani wrote:
The virDomainDeviceInfo struct is defined in device_conf, so generic functions that operate on it should also be defined there rather than in domain_conf.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/device_conf.c | 109 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/device_conf.h | 8 ++++ src/conf/domain_conf.c | 114 ----------------------------------------------- src/conf/domain_conf.h | 7 --- src/libvirt_private.syms | 4 +- 5 files changed, 119 insertions(+), 123 deletions(-)
ACK
It allocates and initializes a virDomainDeviceInfo struct in one fell swoop. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/device_conf.c | 13 +++++++++++++ src/conf/device_conf.h | 1 + 2 files changed, 14 insertions(+) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 4644580..6ead830 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -32,6 +32,19 @@ #define VIR_FROM_THIS VIR_FROM_DEVICE +virDomainDeviceInfoPtr +virDomainDeviceInfoNew(void) +{ + virDomainDeviceInfoPtr info; + + if (VIR_ALLOC(info) < 0) + return NULL; + + virDomainDeviceInfoClear(info); + + return info; +} + void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) { diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 53abe1b..8f641bc 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -170,6 +170,7 @@ struct _virDomainDeviceInfo { char *loadparm; }; +virDomainDeviceInfoPtr virDomainDeviceInfoNew(void); void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, virDomainDeviceInfoPtr src); -- 2.7.5
On Thu, Jun 29, 2017 at 20:03:59 +0200, Andrea Bolognani wrote:
It allocates and initializes a virDomainDeviceInfo struct in one fell swoop.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/device_conf.c | 13 +++++++++++++ src/conf/device_conf.h | 1 + 2 files changed, 14 insertions(+)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 4644580..6ead830 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -32,6 +32,19 @@
#define VIR_FROM_THIS VIR_FROM_DEVICE
+virDomainDeviceInfoPtr +virDomainDeviceInfoNew(void) +{ + virDomainDeviceInfoPtr info; + + if (VIR_ALLOC(info) < 0) + return NULL; + + virDomainDeviceInfoClear(info);
This is a lot of duplicated 0 setting. Given a quick scroll to the other *New function ... we only set parameters which are non-null. The only code path calling the clearing fucntion which is not in a fucntion which then frees the object completely is in virDomainDeviceInfoParseXML. I don't think we need to do it here and additionally the objects are not reused thus you should not need to set anything non-null anywhere else. ACK to the wrapper except for the call to virDomainDeviceInfoClear
Mostly style changes to make them a little bit nicer. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/device_conf.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 6ead830..facde0e 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -45,21 +45,44 @@ virDomainDeviceInfoNew(void) return info; } +/** + * virDomainDeviceInfoClear: + * @info: device info + * + * Reset @info to its default state: all members will be set to their default + * value, and any memory associated with @info will be released. @info itself + * will still be valid after this function returns. + * + * You only need to call this function if you're allocating a + * virDomainDeviceInfo on the stack or embedding one inside your own struct: + * virDomainDeviceInfoNew() already takes care of calling it for you. + */ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) { - VIR_FREE(info->alias); - memset(&info->addr, 0, sizeof(info->addr)); info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; + memset(&info->addr, 0, sizeof(info->addr)); + + VIR_FREE(info->alias); VIR_FREE(info->romfile); VIR_FREE(info->loadparm); } +/** + * virDomainDeviceInfoCopy: + * @dst: destination virDomainDeviceInfo + * @src: source virDomainDeviceInfo + * + * Perform a deep copy of @src into @dst. + * + * Return: 0 on success, <0 on failure + */ int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, virDomainDeviceInfoPtr src) { - /* Assume that dst is already cleared */ + /* Clear the destination */ + virDomainDeviceInfoClear(dst); /* first a shallow copy of *everything* */ *dst = *src; @@ -77,10 +100,11 @@ virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, void virDomainDeviceInfoFree(virDomainDeviceInfoPtr info) { - if (info) { - virDomainDeviceInfoClear(info); - VIR_FREE(info); - } + if (!info) + return; + + virDomainDeviceInfoClear(info); + VIR_FREE(info); } bool -- 2.7.5
On Thu, Jun 29, 2017 at 20:04:00 +0200, Andrea Bolognani wrote:
Mostly style changes to make them a little bit nicer.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/device_conf.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 6ead830..facde0e 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -45,21 +45,44 @@ virDomainDeviceInfoNew(void) return info; }
+/** + * virDomainDeviceInfoClear: + * @info: device info + * + * Reset @info to its default state: all members will be set to their default + * value, and any memory associated with @info will be released. @info itself + * will still be valid after this function returns.
This is not true at this point: @mastertype, @master, @rombar @bootIndex and @pciConnectFlags are not touched here, so you still rely on some clearing beforehand. This function is clearly meant just to release any allocated memory as most of our Clear functions. If you are going to sell it as a full-purposed cleaning functions, you need to scrub everything.
+ * + * You only need to call this function if you're allocating a + * virDomainDeviceInfo on the stack or embedding one inside your own struct: + * virDomainDeviceInfoNew() already takes care of calling it for you. + */ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) { - VIR_FREE(info->alias); - memset(&info->addr, 0, sizeof(info->addr)); info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; + memset(&info->addr, 0, sizeof(info->addr)); + + VIR_FREE(info->alias); VIR_FREE(info->romfile); VIR_FREE(info->loadparm); }
+/** + * virDomainDeviceInfoCopy: + * @dst: destination virDomainDeviceInfo + * @src: source virDomainDeviceInfo + * + * Perform a deep copy of @src into @dst. + * + * Return: 0 on success, <0 on failure + */ int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, virDomainDeviceInfoPtr src) { - /* Assume that dst is already cleared */ + /* Clear the destination */ + virDomainDeviceInfoClear(dst);
e.g. this would be misleading, since it is not fully cleared ...
/* first a shallow copy of *everything* */ *dst = *src;
but thankfully fully overwritten ... Rest looks good though
Instances allocated on the stack or using VIR_ALLOC() directly skip the initialization process. Use the proper allocation function instead to ensure all virDomainDeviceInfo instances are properly initialized. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 6 ++++-- src/conf/domain_conf.c | 4 ++-- src/libvirt_private.syms | 2 ++ src/qemu/qemu_domain_address.c | 29 +++++++++++++++++------------ 4 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 639168e..0c6e8c3 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1427,8 +1427,8 @@ virDomainVirtioSerialAddrAssign(virDomainDefPtr def, bool portOnly) { int ret = -1; - virDomainDeviceInfo nfo = { NULL }; - virDomainDeviceInfoPtr ptr = allowZero ? &nfo : info; + virDomainDeviceInfoPtr nfo = virDomainDeviceInfoNew(); + virDomainDeviceInfoPtr ptr = allowZero ? nfo : info; ptr->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; @@ -1448,6 +1448,8 @@ virDomainVirtioSerialAddrAssign(virDomainDefPtr def, ret = 0; cleanup: + virDomainDeviceInfoFree(nfo); + return ret; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6a89fab..11c4627 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2364,7 +2364,7 @@ virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt) if (VIR_ALLOC(def) < 0) return NULL; - if (VIR_ALLOC(def->info) < 0) + if (!(def->info = virDomainDeviceInfoNew())) goto error; if (xmlopt && @@ -2376,7 +2376,7 @@ virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt) return def; error: - VIR_FREE(def->info); + virDomainDeviceInfoFree(def->info); VIR_FREE(def); goto cleanup; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 178aefc..48e9e33 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -91,6 +91,8 @@ virCPUModeTypeToString; # conf/device_conf.h virDomainDeviceInfoAddressIsEqual; virDomainDeviceInfoCopy; +virDomainDeviceInfoFree; +virDomainDeviceInfoNew; virInterfaceLinkFormat; virInterfaceLinkParseXML; virPCIDeviceAddressEqual; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index b5b863f..93ab304 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1983,6 +1983,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, int ret = -1; virDomainPCIAddressSetPtr addrs = NULL; qemuDomainObjPrivatePtr priv = NULL; + virDomainDeviceInfoPtr info = virDomainDeviceInfoNew(); int max_idx = -1; int nbuses = 0; size_t i; @@ -2031,16 +2032,15 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, */ if (qemuDomainHasPCIRoot(def)) { - /* This is a dummy info used to reserve a slot for a + bool buses_reserved = true; + + /* The dummy info is used to reserve a slot for a * legacy PCI device that doesn't exist, but may in the * future, e.g. if another device is hotplugged into the * domain. */ - virDomainDeviceInfo info = { - .pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | - VIR_PCI_CONNECT_TYPE_PCI_DEVICE) - }; - bool buses_reserved = true; + info->pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | + VIR_PCI_CONNECT_TYPE_PCI_DEVICE); for (i = 0; i < addrs->nbuses; i++) { if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i])) { @@ -2049,7 +2049,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, } } if (!buses_reserved && - qemuDomainPCIAddressReserveNextAddr(addrs, &info) < 0) + qemuDomainPCIAddressReserveNextAddr(addrs, info) < 0) goto cleanup; } @@ -2073,15 +2073,19 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (max_idx <= 0 && addrs->nbuses > max_idx + 1 && qemuDomainHasPCIeRoot(def)) { - virDomainDeviceInfo info = { - .pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | - VIR_PCI_CONNECT_TYPE_PCIE_DEVICE) - }; + + /* The dummy info is used to reserve a slot for a + * PCI Express device that doesn't exist, but may in the + * future, e.g. if another device is hotplugged into the + * domain. + */ + info->pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | + VIR_PCI_CONNECT_TYPE_PCIE_DEVICE); /* if there isn't an empty pcie-root-port, this will * cause one to be added */ - if (qemuDomainPCIAddressReserveNextAddr(addrs, &info) < 0) + if (qemuDomainPCIAddressReserveNextAddr(addrs, info) < 0) goto cleanup; } @@ -2234,6 +2238,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, ret = 0; cleanup: + virDomainDeviceInfoFree(info); virDomainPCIAddressSetFree(addrs); return ret; -- 2.7.5
On Thu, Jun 29, 2017 at 20:04:01 +0200, Andrea Bolognani wrote:
Instances allocated on the stack or using VIR_ALLOC() directly skip the initialization process. Use the proper allocation function instead to ensure all virDomainDeviceInfo instances are properly initialized.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 6 ++++-- src/conf/domain_conf.c | 4 ++-- src/libvirt_private.syms | 2 ++ src/qemu/qemu_domain_address.c | 29 +++++++++++++++++------------ 4 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 178aefc..48e9e33 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -91,6 +91,8 @@ virCPUModeTypeToString; # conf/device_conf.h virDomainDeviceInfoAddressIsEqual; virDomainDeviceInfoCopy; +virDomainDeviceInfoFree; +virDomainDeviceInfoNew;
Looks like this belongs more like it belongs to the patch moving the functions.
virInterfaceLinkFormat; virInterfaceLinkParseXML; virPCIDeviceAddressEqual; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index b5b863f..93ab304 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1983,6 +1983,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, int ret = -1; virDomainPCIAddressSetPtr addrs = NULL; qemuDomainObjPrivatePtr priv = NULL; + virDomainDeviceInfoPtr info = virDomainDeviceInfoNew(); int max_idx = -1; int nbuses = 0; size_t i; @@ -2031,16 +2032,15 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, */
if (qemuDomainHasPCIRoot(def)) { - /* This is a dummy info used to reserve a slot for a + bool buses_reserved = true; + + /* The dummy info is used to reserve a slot for a * legacy PCI device that doesn't exist, but may in the * future, e.g. if another device is hotplugged into the * domain. */ - virDomainDeviceInfo info = { - .pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | - VIR_PCI_CONNECT_TYPE_PCI_DEVICE) - }; - bool buses_reserved = true; + info->pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
@info may be NULL here.
+ VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
for (i = 0; i < addrs->nbuses; i++) { if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i])) { @@ -2049,7 +2049,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, } } if (!buses_reserved && - qemuDomainPCIAddressReserveNextAddr(addrs, &info) < 0) + qemuDomainPCIAddressReserveNextAddr(addrs, info) < 0) goto cleanup; }
@@ -2073,15 +2073,19 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (max_idx <= 0 && addrs->nbuses > max_idx + 1 && qemuDomainHasPCIeRoot(def)) { - virDomainDeviceInfo info = { - .pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | - VIR_PCI_CONNECT_TYPE_PCIE_DEVICE) - }; + + /* The dummy info is used to reserve a slot for a + * PCI Express device that doesn't exist, but may in the + * future, e.g. if another device is hotplugged into the + * domain. + */ + info->pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
Here too.
+ VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);
/* if there isn't an empty pcie-root-port, this will * cause one to be added */ - if (qemuDomainPCIAddressReserveNextAddr(addrs, &info) < 0) + if (qemuDomainPCIAddressReserveNextAddr(addrs, info) < 0) goto cleanup; }
ACK if you make sure that @info is valid, which kind of wasn't necessary with stack alloc'd variables.
Same as virDomainDeviceInfo itself, any struct that embeds it needs to be initialized properly before use; however, none of the structs in question even had a proper allocation function defined. Implement an allocation function for all structs embedding a virDomainDeviceInfo and use them instead of plain VIR_ALLOC() everywhere. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/bhyve/bhyve_parse_command.c | 4 +- src/conf/domain_conf.c | 280 +++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 15 +++ src/libvirt_private.syms | 11 ++ src/openvz/openvz_conf.c | 2 +- src/qemu/qemu_command.c | 12 +- src/qemu/qemu_domain.c | 11 +- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_hotplug.c | 5 +- src/qemu/qemu_parse_command.c | 27 ++-- src/vbox/vbox_common.c | 12 +- src/vmx/vmx.c | 2 +- src/vz/vz_sdk.c | 6 +- src/xen/xen_driver.c | 2 +- src/xenapi/xenapi_driver.c | 2 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 8 +- src/xenconfig/xen_xl.c | 2 +- src/xenconfig/xen_xm.c | 2 +- 19 files changed, 303 insertions(+), 104 deletions(-) diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c index fcaaed2..b9e8bc6 100644 --- a/src/bhyve/bhyve_parse_command.c +++ b/src/bhyve/bhyve_parse_command.c @@ -432,7 +432,7 @@ bhyveParsePCIDisk(virDomainDefPtr def, int idx = -1; virDomainDiskDefPtr disk = NULL; - if (VIR_ALLOC(disk) < 0) + if (!(disk = virDomainDiskDefNew(NULL))) goto cleanup; if (VIR_ALLOC(disk->src) < 0) goto error; @@ -505,7 +505,7 @@ bhyveParsePCINet(virDomainDefPtr def, const char *separator = NULL; const char *mac = NULL; - if (VIR_ALLOC(net) < 0) + if (!(net = virDomainNetDefNew())) goto cleanup; /* As we only support interface type='bridge' and cannot diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 11c4627..055fde9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1389,6 +1389,17 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) VIR_FREE(def); } +virDomainInputDefPtr +virDomainInputDefNew(void) +{ + virDomainInputDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainInputDefFree(virDomainInputDefPtr def) { if (!def) @@ -2021,6 +2032,17 @@ virDomainNetDefClear(virDomainNetDefPtr def) virNetDevVlanClear(&def->vlan); } +virDomainNetDefPtr +virDomainNetDefNew(void) +{ + virDomainNetDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainNetDefFree(virDomainNetDefPtr def) { @@ -2248,6 +2270,17 @@ void virDomainChrDefFree(virDomainChrDefPtr def) VIR_FREE(def); } +virDomainSmartcardDefPtr +virDomainSmartcardDefNew(void) +{ + virDomainSmartcardDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def) { size_t i; @@ -2285,6 +2318,17 @@ void virDomainSoundCodecDefFree(virDomainSoundCodecDefPtr def) VIR_FREE(def); } +virDomainSoundDefPtr +virDomainSoundDefNew(void) +{ + virDomainSoundDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainSoundDefFree(virDomainSoundDefPtr def) { if (!def) @@ -2300,6 +2344,17 @@ void virDomainSoundDefFree(virDomainSoundDefPtr def) VIR_FREE(def); } +virDomainMemballoonDefPtr +virDomainMemballoonDefNew(void) +{ + virDomainMemballoonDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def) { if (!def) @@ -2311,6 +2366,17 @@ void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def) VIR_FREE(def); } +virDomainNVRAMDefPtr +virDomainNVRAMDefNew(void) +{ + virDomainNVRAMDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def) { if (!def) @@ -2321,6 +2387,17 @@ void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def) VIR_FREE(def); } +virDomainWatchdogDefPtr +virDomainWatchdogDefNew(void) +{ + virDomainWatchdogDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def) { if (!def) @@ -2331,6 +2408,50 @@ void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def) VIR_FREE(def); } +virDomainRNGDefPtr +virDomainRNGDefNew(void) +{ + virDomainRNGDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + +void +virDomainRNGDefFree(virDomainRNGDefPtr def) +{ + if (!def) + return; + + switch ((virDomainRNGBackend) def->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + VIR_FREE(def->source.file); + break; + case VIR_DOMAIN_RNG_BACKEND_EGD: + virDomainChrSourceDefFree(def->source.chardev); + break; + case VIR_DOMAIN_RNG_BACKEND_LAST: + break; + } + + virDomainDeviceInfoClear(&def->info); + VIR_FREE(def->virtio); + VIR_FREE(def); +} + +virDomainShmemDefPtr +virDomainShmemDefNew(void) +{ + virDomainShmemDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainShmemDefFree(virDomainShmemDefPtr def) { if (!def) @@ -2342,6 +2463,17 @@ void virDomainShmemDefFree(virDomainShmemDefPtr def) VIR_FREE(def); } +virDomainVideoDefPtr +virDomainVideoDefNew(void) +{ + virDomainVideoDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainVideoDefFree(virDomainVideoDefPtr def) { if (!def) @@ -2457,6 +2589,17 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def) def->privateData = NULL; } +virDomainTPMDefPtr +virDomainTPMDefNew(void) +{ + virDomainTPMDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainTPMDefFree(virDomainTPMDefPtr def) { if (!def) @@ -2489,6 +2632,17 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def) VIR_FREE(def); } +virDomainHubDefPtr +virDomainHubDefNew(void) +{ + virDomainHubDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainHubDefFree(virDomainHubDefPtr def) { if (!def) @@ -2498,6 +2652,17 @@ void virDomainHubDefFree(virDomainHubDefPtr def) VIR_FREE(def); } +virDomainRedirdevDefPtr +virDomainRedirdevDefNew(void) +{ + virDomainRedirdevDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def) { if (!def) @@ -2523,6 +2688,17 @@ void virDomainRedirFilterDefFree(virDomainRedirFilterDefPtr def) VIR_FREE(def); } +virDomainMemoryDefPtr +virDomainMemoryDefNew(void) +{ + virDomainMemoryDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainMemoryDefFree(virDomainMemoryDefPtr def) { if (!def) @@ -2733,6 +2909,17 @@ virDomainResourceDefFree(virDomainResourceDefPtr resource) VIR_FREE(resource); } +virDomainPanicDefPtr +virDomainPanicDefNew(void) +{ + virDomainPanicDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + void virDomainPanicDefFree(virDomainPanicDefPtr panic) { @@ -9732,8 +9919,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr oldnode = ctxt->node; int rv, val; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainNetDefNew())) + goto error; ctxt->node = node; @@ -11198,8 +11385,8 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, virDomainSmartcardDefPtr def; size_t i; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainSmartcardDefNew())) + goto error; mode = virXMLPropString(node, "mode"); if (mode == NULL) { @@ -11344,8 +11531,8 @@ virDomainTPMDefParseXML(xmlNodePtr node, xmlNodePtr *backends = NULL; int nbackends; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainTPMDefNew())) + goto error; model = virXMLPropString(node, "model"); if (model != NULL && @@ -11425,8 +11612,8 @@ virDomainPanicDefParseXML(xmlNodePtr node, virDomainPanicDefPtr panic; char *model = NULL; - if (VIR_ALLOC(panic) < 0) - return NULL; + if (!(panic = virDomainPanicDefNew())) + goto error; if (virDomainDeviceInfoParseXML(node, NULL, &panic->info, flags) < 0) goto error; @@ -11462,8 +11649,8 @@ virDomainInputDefParseXML(const virDomainDef *dom, char *type = NULL; char *bus = NULL; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainInputDefNew())) + goto error; ctxt->node = node; @@ -11606,8 +11793,8 @@ virDomainHubDefParseXML(xmlNodePtr node, unsigned int flags) virDomainHubDefPtr def; char *type = NULL; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainHubDefNew())) + goto error; type = virXMLPropString(node, "type"); @@ -12708,8 +12895,8 @@ virDomainSoundDefParseXML(xmlNodePtr node, virDomainSoundDefPtr def; xmlNodePtr save = ctxt->node; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainSoundDefNew())) + goto error; ctxt->node = node; @@ -12777,8 +12964,8 @@ virDomainWatchdogDefParseXML(xmlNodePtr node, char *action = NULL; virDomainWatchdogDefPtr def; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainWatchdogDefNew())) + goto error; model = virXMLPropString(node, "model"); if (model == NULL) { @@ -12835,8 +13022,8 @@ virDomainRNGDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr *backends = NULL; int nbackends; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainRNGDefNew())) + goto error; if (!(model = virXMLPropString(node, "model"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing RNG device model")); @@ -12949,8 +13136,8 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, xmlNodePtr save = ctxt->node; unsigned int period = 0; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainMemballoonDefNew())) + goto error; model = virXMLPropString(node, "model"); if (model == NULL) { @@ -13010,8 +13197,8 @@ virDomainNVRAMDefParseXML(xmlNodePtr node, { virDomainNVRAMDefPtr def; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainNVRAMDefNew())) + goto error; if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; @@ -13034,9 +13221,8 @@ virDomainShmemDefParseXML(xmlNodePtr node, xmlNodePtr save = ctxt->node; xmlNodePtr server = NULL; - - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainShmemDefNew())) + goto error; ctxt->node = node; @@ -13550,8 +13736,8 @@ virDomainVideoDefParseXML(xmlNodePtr node, ctxt->node = node; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainVideoDefNew())) + goto error; cur = node->children; while (cur != NULL) { @@ -13789,8 +13975,8 @@ virDomainRedirdevDefParseXML(virDomainXMLOptionPtr xmlopt, char *bus = NULL, *type = NULL; int remaining; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainRedirdevDefNew())) + goto error; if (!(def->source = virDomainChrSourceDefNew(xmlopt))) goto error; @@ -14255,8 +14441,8 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode, ctxt->node = memdevNode; - if (VIR_ALLOC(def) < 0) - return NULL; + if (!(def = virDomainMemoryDefNew())) + goto error; if (!(tmp = virXMLPropString(memdevNode, "model"))) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -16383,7 +16569,7 @@ virDomainDefAddController(virDomainDefPtr def, int type, int idx, int model) cont->model = model; if (VIR_APPEND_ELEMENT_COPY(def->controllers, def->ncontrollers, cont) < 0) { - VIR_FREE(cont); + virDomainControllerDefFree(cont); return NULL; } @@ -16477,14 +16663,14 @@ virDomainDefMaybeAddInput(virDomainDefPtr def, return 0; } - if (VIR_ALLOC(input) < 0) + if (!(input = virDomainInputDefNew())) return -1; input->type = type; input->bus = bus; if (VIR_APPEND_ELEMENT(def->inputs, def->ninputs, input) < 0) { - VIR_FREE(input); + virDomainInputDefFree(input); return -1; } @@ -20827,14 +21013,14 @@ static int virDomainDefAddImplicitVideo(virDomainDefPtr def) { int ret = -1; - virDomainVideoDefPtr video = NULL; + virDomainVideoDefPtr video; /* For backwards compatibility, if no <video> tag is set but there * is a <graphics> tag, then we add a single video tag */ if (def->ngraphics == 0 || def->nvideos > 0) return 0; - if (VIR_ALLOC(video) < 0) + if (!(video = virDomainVideoDefNew())) goto cleanup; video->type = virDomainVideoDefaultType(def); if (video->type < 0) { @@ -23253,28 +23439,6 @@ virDomainRNGDefFormat(virBufferPtr buf, return 0; } -void -virDomainRNGDefFree(virDomainRNGDefPtr def) -{ - if (!def) - return; - - switch ((virDomainRNGBackend) def->backend) { - case VIR_DOMAIN_RNG_BACKEND_RANDOM: - VIR_FREE(def->source.file); - break; - case VIR_DOMAIN_RNG_BACKEND_EGD: - virDomainChrSourceDefFree(def->source.chardev); - break; - case VIR_DOMAIN_RNG_BACKEND_LAST: - break; - } - - virDomainDeviceInfoClear(&def->info); - VIR_FREE(def->virtio); - VIR_FREE(def); -} - static int virDomainMemorySourceDefFormat(virBufferPtr buf, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 00d0d65..a09669a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2067,6 +2067,7 @@ struct _virDomainMemoryDef { virDomainDeviceInfo info; }; +virDomainMemoryDefPtr virDomainMemoryDefNew(void); void virDomainMemoryDefFree(virDomainMemoryDefPtr def); struct _virDomainIdMapEntry { @@ -2641,9 +2642,11 @@ int virDomainObjWait(virDomainObjPtr vm); int virDomainObjWaitUntil(virDomainObjPtr vm, unsigned long long whenms); +virDomainPanicDefPtr virDomainPanicDefNew(void); void virDomainPanicDefFree(virDomainPanicDefPtr panic); void virDomainResourceDefFree(virDomainResourceDefPtr resource); void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def); +virDomainInputDefPtr virDomainInputDefNew(void); void virDomainInputDefFree(virDomainInputDefPtr def); virDomainDiskDefPtr virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt); void virDomainDiskDefFree(virDomainDiskDefPtr def); @@ -2672,24 +2675,34 @@ virDomainControllerDefNew(virDomainControllerType type); void virDomainFSDefFree(virDomainFSDefPtr def); void virDomainActualNetDefFree(virDomainActualNetDefPtr def); void virDomainNetDefClear(virDomainNetDefPtr def); +virDomainNetDefPtr virDomainNetDefNew(void); void virDomainNetDefFree(virDomainNetDefPtr def); +virDomainSmartcardDefPtr virDomainSmartcardDefNew(void); void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def); void virDomainChrDefFree(virDomainChrDefPtr def); void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def); int virDomainChrSourceDefCopy(virDomainChrSourceDefPtr src, virDomainChrSourceDefPtr dest); void virDomainSoundCodecDefFree(virDomainSoundCodecDefPtr def); +virDomainSoundDefPtr virDomainSoundDefNew(void); void virDomainSoundDefFree(virDomainSoundDefPtr def); +virDomainMemballoonDefPtr virDomainMemballoonDefNew(void); void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def); +virDomainNVRAMDefPtr virDomainNVRAMDefNew(void); void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def); +virDomainWatchdogDefPtr virDomainWatchdogDefNew(void); void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); +virDomainVideoDefPtr virDomainVideoDefNew(void); void virDomainVideoDefFree(virDomainVideoDefPtr def); virDomainHostdevDefPtr virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt); void virDomainHostdevDefClear(virDomainHostdevDefPtr def); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); +virDomainHubDefPtr virDomainHubDefNew(void); void virDomainHubDefFree(virDomainHubDefPtr def); +virDomainRedirdevDefPtr virDomainRedirdevDefNew(void); void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def); void virDomainRedirFilterDefFree(virDomainRedirFilterDefPtr def); +virDomainShmemDefPtr virDomainShmemDefNew(void); void virDomainShmemDefFree(virDomainShmemDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src, @@ -2699,6 +2712,7 @@ virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src, int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int type); virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device); +virDomainTPMDefPtr virDomainTPMDefNew(void); void virDomainTPMDefFree(virDomainTPMDefPtr def); typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, @@ -2903,6 +2917,7 @@ int virDomainDefCompatibleDevice(virDomainDefPtr def, virDomainDeviceDefPtr dev, virDomainDeviceAction action); +virDomainRNGDefPtr virDomainRNGDefNew(void); void virDomainRNGDefFree(virDomainRNGDefPtr def); int virDomainDiskIndexByAddress(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 48e9e33..98f3969 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -377,11 +377,13 @@ virDomainHostdevModeTypeToString; virDomainHostdevRemove; virDomainHostdevSubsysPCIBackendTypeToString; virDomainHostdevSubsysTypeToString; +virDomainHubDefNew; virDomainHubTypeFromString; virDomainHubTypeToString; virDomainHypervTypeFromString; virDomainHypervTypeToString; virDomainInputDefFree; +virDomainInputDefNew; virDomainIOMMUModelTypeFromString; virDomainIOMMUModelTypeToString; virDomainIOThreadIDAdd; @@ -406,9 +408,11 @@ virDomainLoaderTypeFromString; virDomainLoaderTypeToString; virDomainLockFailureTypeFromString; virDomainLockFailureTypeToString; +virDomainMemballoonDefNew; virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainMemoryDefFree; +virDomainMemoryDefNew; virDomainMemoryFindByDef; virDomainMemoryFindInactiveByDef; virDomainMemoryInsert; @@ -419,6 +423,7 @@ virDomainNetAppendIPAddress; virDomainNetDefClear; virDomainNetDefFormat; virDomainNetDefFree; +virDomainNetDefNew; virDomainNetFind; virDomainNetFindIdx; virDomainNetGenerateMAC; @@ -439,6 +444,7 @@ virDomainNetTypeFromString; virDomainNetTypeToString; virDomainNostateReasonTypeFromString; virDomainNostateReasonTypeToString; +virDomainNVRAMDefNew; virDomainObjAssignDef; virDomainObjBroadcast; virDomainObjCopyPersistentDef; @@ -463,6 +469,8 @@ virDomainObjWait; virDomainObjWaitUntil; virDomainOSTypeFromString; virDomainOSTypeToString; +virDomainPanicDefFree; +virDomainPanicDefNew; virDomainParseMemory; virDomainPausedReasonTypeFromString; virDomainPausedReasonTypeToString; @@ -503,6 +511,7 @@ virDomainSmartcardTypeToString; virDomainSmbiosModeTypeFromString; virDomainSmbiosModeTypeToString; virDomainSoundDefFree; +virDomainSoundDefNew; virDomainSoundModelTypeFromString; virDomainSoundModelTypeToString; virDomainStartupPolicyTypeFromString; @@ -530,6 +539,7 @@ virDomainUSBDeviceDefForeach; virDomainVideoDefaultRAM; virDomainVideoDefaultType; virDomainVideoDefFree; +virDomainVideoDefNew; virDomainVideoTypeFromString; virDomainVideoTypeToString; virDomainVideoVGAConfTypeFromString; @@ -538,6 +548,7 @@ virDomainVirtTypeFromString; virDomainVirtTypeToString; virDomainWatchdogActionTypeFromString; virDomainWatchdogActionTypeToString; +virDomainWatchdogDefNew; virDomainWatchdogModelTypeFromString; virDomainWatchdogModelTypeToString; virDomainXMLOptionGetNamespace; diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 23a02d7..a551229 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -219,7 +219,7 @@ openvzReadNetworkConf(virDomainDefPtr def, } else if (ret > 0) { token = strtok_r(temp, " ", &saveptr); while (token != NULL) { - if (VIR_ALLOC(net) < 0) + if (!(net = virDomainNetDefNew())) goto error; net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c53ab97..43a23d1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3522,19 +3522,22 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, const char *backendType; int ret = -1; int rc; - virDomainMemoryDef mem = { 0 }; + virDomainMemoryDefPtr mem; unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa, cell); + if (!(mem = virDomainMemoryDefNew())) + goto cleanup; + *backendStr = NULL; - mem.size = memsize; - mem.targetNode = cell; + mem->size = memsize; + mem->targetNode = cell; if (virAsprintf(&alias, "ram-node%zu", cell) < 0) goto cleanup; if ((rc = qemuBuildMemoryBackendStr(&props, &backendType, cfg, qemuCaps, - def, &mem, auto_nodeset, false)) < 0) + def, mem, auto_nodeset, false)) < 0) goto cleanup; if (!(*backendStr = virQEMUBuildObjectCommandlineFromJSON(backendType, @@ -3547,6 +3550,7 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, cleanup: VIR_FREE(alias); virJSONValueFree(props); + virDomainMemoryDefFree(mem); return ret; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8e7404d..451bbdf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2511,7 +2511,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, if (addDefaultMemballoon && !def->memballoon) { virDomainMemballoonDefPtr memballoon; - if (VIR_ALLOC(memballoon) < 0) + if (!(memballoon = virDomainMemballoonDefNew())) goto cleanup; memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO; @@ -2545,10 +2545,13 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, if (j == def->npanics) { virDomainPanicDefPtr panic; - if (VIR_ALLOC(panic) < 0 || - VIR_APPEND_ELEMENT_COPY(def->panics, + + if (!(panic = virDomainPanicDefNew())) + goto cleanup; + + if (VIR_APPEND_ELEMENT_COPY(def->panics, def->npanics, panic) < 0) { - VIR_FREE(panic); + virDomainPanicDefFree(panic); goto cleanup; } } diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 93ab304..c827a7e 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -2342,7 +2342,7 @@ qemuDomainUSBAddressAddHubs(virDomainDefPtr def) data.count, available_ports, hubs_needed); for (i = 0; i < hubs_needed; i++) { - if (VIR_ALLOC(hub) < 0) + if (!(hub = virDomainHubDefNew())) return -1; hub->type = VIR_DOMAIN_HUB_TYPE_USB; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b5b62df..735983a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -572,16 +572,15 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, /* No SCSI controller present, for backward compatibility we * now hotplug a controller */ - if (VIR_ALLOC(cont) < 0) + if (!(cont = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_SCSI))) return NULL; - cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; cont->idx = controller; cont->model = -1; VIR_INFO("No SCSI controller present, hotplugging one"); if (qemuDomainAttachControllerDevice(driver, vm, cont) < 0) { - VIR_FREE(cont); + virDomainControllerDefFree(cont); return NULL; } diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index 90e8d09..3d1f058 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -649,7 +649,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, 0) < 0) return NULL; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDiskDefNew(NULL))) goto cleanup; if (VIR_ALLOC(def->src) < 0) goto error; @@ -1032,7 +1032,7 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, nkeywords = 0; } - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainNetDefNew())) goto cleanup; /* 'tap' could turn into libvirt type=ethernet, type=bridge or @@ -1502,10 +1502,13 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, if (j == dom->npanics) { virDomainPanicDefPtr panic; - if (VIR_ALLOC(panic) < 0 || - VIR_APPEND_ELEMENT_COPY(dom->panics, + + if (!(panic = virDomainPanicDefNew())) + goto cleanup; + + if (VIR_APPEND_ELEMENT_COPY(dom->panics, dom->npanics, panic) < 0) { - VIR_FREE(panic); + virDomainPanicDefFree(panic); goto cleanup; } panic->model = VIR_DOMAIN_PANIC_MODEL_HYPERV; @@ -2240,7 +2243,7 @@ qemuParseCommandLine(virCapsPtr caps, STREQ(val, "mouse") || STREQ(val, "keyboard")) { virDomainInputDefPtr input; - if (VIR_ALLOC(input) < 0) + if (!(input = virDomainInputDefNew())) goto error; input->bus = VIR_DOMAIN_INPUT_BUS_USB; if (STREQ(val, "tablet")) @@ -2330,7 +2333,7 @@ qemuParseCommandLine(virCapsPtr caps, if (type != -1) { virDomainSoundDefPtr snd; - if (VIR_ALLOC(snd) < 0) + if (!(snd = virDomainSoundDefNew())) goto error; snd->model = type; if (VIR_APPEND_ELEMENT(def->sounds, def->nsounds, snd) < 0) { @@ -2347,7 +2350,7 @@ qemuParseCommandLine(virCapsPtr caps, if (model != -1) { virDomainWatchdogDefPtr wd; - if (VIR_ALLOC(wd) < 0) + if (!(wd = virDomainWatchdogDefNew())) goto error; wd->model = model; wd->action = VIR_DOMAIN_WATCHDOG_ACTION_RESET; @@ -2450,7 +2453,7 @@ qemuParseCommandLine(virCapsPtr caps, STRPREFIX(progargv[i + 1], "spapr-nvram.reg=")) { WANT_VALUE(); - if (VIR_ALLOC(def->nvram) < 0) + if (!(def->nvram = virDomainNVRAMDefNew())) goto error; def->nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; @@ -2477,7 +2480,7 @@ qemuParseCommandLine(virCapsPtr caps, if (STRPREFIX(opts, "virtio-balloon")) { WANT_VALUE(); - if (VIR_ALLOC(def->memballoon) < 0) + if (!(def->memballoon = virDomainMemballoonDefNew())) goto error; def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO; } else { @@ -2606,7 +2609,7 @@ qemuParseCommandLine(virCapsPtr caps, if (def->ngraphics) { virDomainVideoDefPtr vid; - if (VIR_ALLOC(vid) < 0) + if (!(vid = virDomainVideoDefNew())) goto error; if (def->virtType == VIR_DOMAIN_VIRT_XEN) vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN; @@ -2631,7 +2634,7 @@ qemuParseCommandLine(virCapsPtr caps, */ if (!def->memballoon) { virDomainMemballoonDefPtr memballoon; - if (VIR_ALLOC(memballoon) < 0) + if (!(memballoon = virDomainMemballoonDefNew())) goto error; memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE; diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 92ee371..32f45c3 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3223,7 +3223,7 @@ vboxDumpVideo(virDomainDefPtr def, vboxDriverPtr data ATTRIBUTE_UNUSED, return -1; def->nvideos = 1; - if (VIR_ALLOC(def->videos[0]) < 0) + if (!(def->videos[0] = virDomainVideoDefNew())) return -1; gVBoxAPI.UIMachine.GetVRAMSize(machine, &VRAMSize); @@ -3390,7 +3390,7 @@ vboxDumpSharedFolders(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine char *hostPath = NULL; PRBool writable = PR_FALSE; - if (VIR_ALLOC(def->fss[i]) < 0) + if (!(def->fss[i] = virDomainFSDefNew())) goto sharedFoldersCleanup; def->fss[i]->type = VIR_DOMAIN_FS_TYPE_MOUNT; @@ -3451,7 +3451,7 @@ vboxDumpNetwork(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRUi /* Allocate memory for the networkcards which are enabled */ if ((def->nnets > 0) && (VIR_ALLOC_N(def->nets, def->nnets) >= 0)) { for (i = 0; i < def->nnets; i++) - ignore_value(VIR_ALLOC(def->nets[i])); + def->nets[i] = virDomainNetDefNew(); } /* Now get the details about the network cards here */ @@ -3585,7 +3585,7 @@ vboxDumpAudio(virDomainDefPtr def, vboxDriverPtr data ATTRIBUTE_UNUSED, def->nsounds = 1; if (VIR_ALLOC_N(def->sounds, def->nsounds) >= 0) { - if (VIR_ALLOC(def->sounds[0]) >= 0) { + if ((def->sounds[0] = virDomainSoundDefNew())) { gVBoxAPI.UIAudioAdapter.GetAudioController(audioAdapter, &audioController); if (audioController == AudioControllerType_SB16) { def->sounds[0]->model = VIR_DOMAIN_SOUND_MODEL_SB16; @@ -3630,7 +3630,7 @@ vboxDumpSerial(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRUin /* Allocate memory for the serial ports which are enabled */ if ((def->nserials > 0) && (VIR_ALLOC_N(def->serials, def->nserials) >= 0)) { for (i = 0; i < def->nserials; i++) - ignore_value(VIR_ALLOC(def->serials[i])); + def->serials[i] = virDomainChrDefNew(NULL); } /* Now get the details about the serial ports here */ @@ -3718,7 +3718,7 @@ vboxDumpParallel(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRU /* Allocate memory for the parallel ports which are enabled */ if ((def->nparallels > 0) && (VIR_ALLOC_N(def->parallels, def->nparallels) >= 0)) { for (i = 0; i < def->nparallels; i++) - ignore_value(VIR_ALLOC(def->parallels[i])); + def->parallels[i] = virDomainChrDefNew(NULL); } /* Now get the details about the parallel ports here */ diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 96507f1..2203599 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -3037,7 +3037,7 @@ virVMXParseSVGA(virConfPtr conf, virDomainVideoDefPtr *def) return -1; } - if (VIR_ALLOC(*def) < 0) + if (!(*def = virDomainVideoDefNew())) return -1; (*def)->type = VIR_DOMAIN_VIDEO_TYPE_VMVGA; diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 0aa1a30..161ce30 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -552,7 +552,7 @@ prlsdkAddDomainVideoInfoCt(virDomainDefPtr def) if (def->ngraphics == 0) return 0; - if (VIR_ALLOC(video) < 0) + if (!(virDomainVideoDefNew())) goto cleanup; video->type = VIR_DOMAIN_VIDEO_TYPE_PARALLELS; @@ -581,7 +581,7 @@ prlsdkAddDomainVideoInfoVm(PRL_HANDLE sdkdom, virDomainDefPtr def) ret = PrlVmCfg_GetVideoRamSize(sdkdom, &videoRam); prlsdkCheckRetGoto(ret, error); - if (VIR_ALLOC(video) < 0) + if (!(video = virDomainVideoDefNew())) goto error; if (VIR_ALLOC(accel) < 0) @@ -1157,7 +1157,7 @@ prlsdkAddDomainNetInfo(PRL_HANDLE sdkdom, virDomainDefPtr def) ret = PrlVmCfg_GetNetAdapter(sdkdom, i, &netAdapter); prlsdkCheckRetGoto(ret, error); - if (VIR_ALLOC(net) < 0) + if (!(net = virDomainNetDefNew())) goto error; if (prlsdkGetNetInfo(netAdapter, net, IS_CT(def)) < 0) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index b5cd47e..53e1ba8 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -375,7 +375,7 @@ xenDomainDefPostParse(virDomainDefPtr def, { if (!def->memballoon) { virDomainMemballoonDefPtr memballoon; - if (VIR_ALLOC(memballoon) < 0) + if (!(memballoon = virDomainMemballoonDefNew())) return -1; memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_XEN; diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index fb462cd..09fbcff 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1548,7 +1548,7 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) goto error; } for (i = 0; i < vif_set->size; i++) { - if (VIR_ALLOC(defPtr->nets[i]) < 0) { + if (!(defPtr->nets[i] = virDomainNetDefNew())) xen_vif_set_free(vif_set); goto error; } diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 6d7dc2c..146acc4 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -942,7 +942,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) key = nextkey; } - if (VIR_ALLOC(net) < 0) + if (!(net = virDomainNetDefNew())) goto cleanup; if (mac[0]) { diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index fefa61a..f37a458 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -605,7 +605,7 @@ xenParseSxprNets(virDomainDefPtr def, model = sexpr_node(node, "device/vif/model"); type = sexpr_node(node, "device/vif/type"); - if (VIR_ALLOC(net) < 0) + if (!(net = virDomainNetDefNew())) goto cleanup; if (tmp != NULL || @@ -728,7 +728,7 @@ xenParseSxprSound(virDomainDefPtr def, for (i = 0; i < (VIR_DOMAIN_SOUND_MODEL_ES1370 + 1); i++) { virDomainSoundDefPtr sound; - if (VIR_ALLOC(sound) < 0) + if (!(sound = virDomainSoundDefNew())) goto error; sound->model = i; def->sounds[def->nsounds++] = sound; @@ -752,7 +752,7 @@ xenParseSxprSound(virDomainDefPtr def, goto error; } - if (VIR_ALLOC(sound) < 0) + if (!(sound = virDomainSoundDefNew())) goto error; if ((sound->model = virDomainSoundModelTypeFromString(model)) < 0) { @@ -801,7 +801,7 @@ xenParseSxprUSB(virDomainDefPtr def, STREQ(tmp, "mouse") || STREQ(tmp, "keyboard")) { virDomainInputDefPtr input; - if (VIR_ALLOC(input) < 0) + if (!(input = virDomainInputDefNew())) goto error; input->bus = VIR_DOMAIN_INPUT_BUS_USB; if (STREQ(tmp, "tablet")) diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index fa3f1d0..86eacf1 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -550,7 +550,7 @@ xenParseXLInputDevs(virConfPtr conf, virDomainDefPtr def) STREQ(str, "mouse") || STREQ(str, "keyboard"))) { virDomainInputDefPtr input; - if (VIR_ALLOC(input) < 0) + if (!(input = virDomainInputDefNew())) return -1; input->bus = VIR_DOMAIN_INPUT_BUS_USB; diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index 8ef68bb..79c7f3b 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -412,7 +412,7 @@ xenParseXMInputDevs(virConfPtr conf, virDomainDefPtr def) STREQ(str, "mouse") || STREQ(str, "keyboard"))) { virDomainInputDefPtr input; - if (VIR_ALLOC(input) < 0) + if (!(input = virDomainInputDefNew())) return -1; input->bus = VIR_DOMAIN_INPUT_BUS_USB; -- 2.7.5
On Thu, Jun 29, 2017 at 08:04:02PM +0200, Andrea Bolognani wrote:
Same as virDomainDeviceInfo itself, any struct that embeds it needs to be initialized properly before use; however, none of the structs in question even had a proper allocation function defined.
Implement an allocation function for all structs embedding a virDomainDeviceInfo and use them instead of plain VIR_ALLOC() everywhere.
NACK This is a pointless obfuscation Jan
On Fri, 2017-06-30 at 10:18 +0200, Ján Tomko wrote:
Same as virDomainDeviceInfo itself, any struct that embeds it needs to be initialized properly before use; however, none of the structs in question even had a proper allocation function defined. Implement an allocation function for all structs embedding a virDomainDeviceInfo and use them instead of plain VIR_ALLOC() everywhere. NACK This is a pointless obfuscation
Would you mind spending a few words to explain why you feel that's the case? Having a function where you perform both allocation and initialization of your data structure is a pretty common pattern both outside and inside libvirt, and while it adds one layer of indirection it also improves flexibility and encapsulation, which makes it IMHO well worth it. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jun 30, 2017 at 12:34:15PM +0200, Andrea Bolognani wrote:
On Fri, 2017-06-30 at 10:18 +0200, Ján Tomko wrote:
Same as virDomainDeviceInfo itself, any struct that embeds it needs to be initialized properly before use; however, none of the structs in question even had a proper allocation function defined. Implement an allocation function for all structs embedding a virDomainDeviceInfo and use them instead of plain VIR_ALLOC() everywhere. NACK This is a pointless obfuscation
Would you mind spending a few words to explain why you feel that's the case?
Having a function where you perform both allocation and initialization of your data structure is a pretty common
As of now, nothing special is initialized in the structure. If you are going to need initialize something in the future, please mention it also in the commit message, not just the cover letter. This makes the obfuscation pointful.
pattern both outside and inside libvirt, and while it adds one layer of indirection it also improves flexibility and encapsulation, which makes it IMHO well worth it.
I am not a fan of adding code just in case we might need it in the future, which is what this patch looks like to a lazy reviewer that does not read cover letters for cleanups/refactors. Jan
On Thu, Jun 29, 2017 at 20:04:02 +0200, Andrea Bolognani wrote:
Same as virDomainDeviceInfo itself, any struct that embeds it needs to be initialized properly before use; however, none of the structs in question even had a proper allocation function defined.
Implement an allocation function for all structs embedding a virDomainDeviceInfo and use them instead of plain VIR_ALLOC() everywhere.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/bhyve/bhyve_parse_command.c | 4 +- src/conf/domain_conf.c | 280 +++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 15 +++ src/libvirt_private.syms | 11 ++ src/openvz/openvz_conf.c | 2 +- src/qemu/qemu_command.c | 12 +- src/qemu/qemu_domain.c | 11 +- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_hotplug.c | 5 +- src/qemu/qemu_parse_command.c | 27 ++-- src/vbox/vbox_common.c | 12 +- src/vmx/vmx.c | 2 +- src/vz/vz_sdk.c | 6 +- src/xen/xen_driver.c | 2 +- src/xenapi/xenapi_driver.c | 2 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 8 +- src/xenconfig/xen_xl.c | 2 +- src/xenconfig/xen_xm.c | 2 +- 19 files changed, 303 insertions(+), 104 deletions(-)
While I agree that having allocation functions which initialize the internals are necessary in some cases, this patch is a mess as you squashed everything into one place.
Any struct embedding a virDomainDeviceInfo needs to initialize it by calling virDomainDeviceInfoClear(). Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 055fde9..fcb8465 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1397,6 +1397,8 @@ virDomainInputDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -1725,6 +1727,8 @@ virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt) if (VIR_ALLOC(ret->src) < 0) goto error; + virDomainDeviceInfoClear(&ret->info); + if (xmlopt && xmlopt->privateData.diskNew && !(ret->privateData = xmlopt->privateData.diskNew())) @@ -1854,6 +1858,8 @@ virDomainControllerDefNew(virDomainControllerType type) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + def->type = type; /* initialize anything that has a non-0 default */ @@ -1910,6 +1916,8 @@ virDomainFSDefNew(void) if (VIR_ALLOC(ret->src) < 0) goto cleanup; + virDomainDeviceInfoClear(&ret->info); + return ret; cleanup: @@ -2040,6 +2048,8 @@ virDomainNetDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2278,6 +2288,8 @@ virDomainSmartcardDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2326,6 +2338,8 @@ virDomainSoundDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2352,6 +2366,8 @@ virDomainMemballoonDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2374,6 +2390,8 @@ virDomainNVRAMDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2395,6 +2413,8 @@ virDomainWatchdogDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2416,6 +2436,8 @@ virDomainRNGDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2449,6 +2471,8 @@ virDomainShmemDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2471,6 +2495,8 @@ virDomainVideoDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2499,6 +2525,8 @@ virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt) if (!(def->info = virDomainDeviceInfoNew())) goto error; + virDomainDeviceInfoClear(def->info); + if (xmlopt && xmlopt->privateData.hostdevNew && !(def->privateData = xmlopt->privateData.hostdevNew())) @@ -2597,6 +2625,8 @@ virDomainTPMDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2640,6 +2670,8 @@ virDomainHubDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2660,6 +2692,8 @@ virDomainRedirdevDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2696,6 +2730,8 @@ virDomainMemoryDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -2917,6 +2953,8 @@ virDomainPanicDefNew(void) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + return def; } @@ -11229,6 +11267,8 @@ virDomainChrDefNew(virDomainXMLOptionPtr xmlopt) if (VIR_ALLOC(def) < 0) return NULL; + virDomainDeviceInfoClear(&def->info); + def->target.port = -1; if (!(def->source = virDomainChrSourceDefNew(xmlopt))) -- 2.7.5
On Thu, 2017-06-29 at 20:03 +0200, Andrea Bolognani wrote:
This series is required to solve a known limitation in the current incarnation of device isolation support[1], namely the inability to isolate host devices coming from IOMMU group 0. The issue lies in the fact that virDomainDeviceInfo, and all virDomain*Def that embed it, are usually allocated through VIR_ALLOC(), which result in all their fields being initialized to zero. That's worked out just fine so far, because zero was a sensible default value for all existing fields; however, when implementing isolation groups, we add a new virDomainDeviceInfo::isolationGroup field which we need to be initialized to -1 instead so that it doesn't overlap with IOMMU group 0 mentioned above.
Or we could just, you know, do the sensible thing and store (IOMMU group + 1) instead of (IOMMU group) in virDomainDeviceInfo::isolationGroup and avoid the issue altogether? I'm actually quite embarassed I didn't think of that earlier :/
Solving the issue involves creating twenty or so virDomain*DefNew() functions and using them instead of VIR_ALLOC() every time a virDomain*Def needs to be created, which is arguably a pretty good idea regardless.
We could still merge this series, though, or at least parts of it. It improves upon some questionable code. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jun 30, 2017 at 10:48:36 +0200, Andrea Bolognani wrote:
On Thu, 2017-06-29 at 20:03 +0200, Andrea Bolognani wrote:
[...]
That's worked out just fine so far, because zero was a sensible default value for all existing fields; however, when implementing isolation groups, we add a new virDomainDeviceInfo::isolationGroup field which we need to be initialized to -1 instead so that it doesn't overlap with IOMMU group 0 mentioned above.
Or we could just, you know, do the sensible thing and store (IOMMU group + 1) instead of (IOMMU group) in
How is that sensible? That looks as a source of bugs in the long run.
On Fri, Jun 30, 2017 at 10:58:42 +0200, Peter Krempa wrote:
On Fri, Jun 30, 2017 at 10:48:36 +0200, Andrea Bolognani wrote:
On Thu, 2017-06-29 at 20:03 +0200, Andrea Bolognani wrote:
[...]
That's worked out just fine so far, because zero was a sensible default value for all existing fields; however, when implementing isolation groups, we add a new virDomainDeviceInfo::isolationGroup field which we need to be initialized to -1 instead so that it doesn't overlap with IOMMU group 0 mentioned above.
Or we could just, you know, do the sensible thing and store (IOMMU group + 1) instead of (IOMMU group) in
How is that sensible? That looks as a source of bugs in the long run.
Btw, isn't there any external detail which would indicate that the IOMMU group is valid? In that case you could use that and read the value only if it's supposed to be meaningful. Or add a boolean which says that it's meaningfull if you are on the lazy side, but storing the group with any offset is weird.
On Fri, 2017-06-30 at 10:58 +0200, Peter Krempa wrote:
Or we could just, you know, do the sensible thing and store (IOMMU group + 1) instead of (IOMMU group) in How is that sensible? That looks as a source of bugs in the long run.
Isolation groups are used to make sure any given device ends up on the same bus as related devices and on a different bus as unrelated devices. They're an abstract concept, and while working on the initial implementation it just happened to be convenient for me to have the isolation group match the IOMMU group. There's no specific reason that has to be the case. We're never converting back and forth between the two, which I agree would end up in misery at some point down the line; we just set the isolation group once per device and then just perform comparison between isolation groups from there on. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jun 30, 2017 at 12:24:33 +0200, Andrea Bolognani wrote:
On Fri, 2017-06-30 at 10:58 +0200, Peter Krempa wrote:
Or we could just, you know, do the sensible thing and store (IOMMU group + 1) instead of (IOMMU group) in How is that sensible? That looks as a source of bugs in the long run.
Isolation groups are used to make sure any given device ends up on the same bus as related devices and on a different bus as unrelated devices.
They're an abstract concept, and while working on the initial implementation it just happened to be convenient for me to have the isolation group match the IOMMU group. There's no specific reason that has to be the case.
Fair enough. The documentation you are adding in the linked series is vague enough to alow this meaning too: @@ -164,6 +164,16 @@ struct _virDomainDeviceInfo { */ int pciConnectFlags; /* enum virDomainPCIConnectFlags */ char *loadparm; + + /* PCI devices will only be automatically placed on a PCI bus + * that shares the same isolation group */ + int isolationGroup; + + /* Usually, PCI buses will take on the same isolation group + * as the first device that is plugged into them, but in some + * cases we might want to prevent that from happening by + * locking the isolation group */ + bool isolationGroupLocked; };
We're never converting back and forth between the two, which I agree would end up in misery at some point down the line; we just set the isolation group once per device and then just perform comparison between isolation groups from there on.
I'd suggest you create a helper to assign those then (be it from IOMMU group or something else), so there's at least a single point that can be referenced in the future and which will explain this reasoning. Also adding a note that 0 means the device is not isolated would make sense in the structure above.
On Fri, 2017-06-30 at 12:38 +0200, Peter Krempa wrote:
> > Isolation groups are used to make sure any given device ends
> > up on the same bus as related devices and on a different bus
> > as unrelated devices.
> >
> > They're an abstract concept, and while working on the initial
> > implementation it just happened to be convenient for me to
> > have the isolation group match the IOMMU group. There's no
> > specific reason that has to be the case.
>
> Fair enough. The documentation you are adding in the linked series is
> vague enough to alow this meaning too:
>
> @@ -164,6 +164,16 @@ struct _virDomainDeviceInfo {
> */
> int pciConnectFlags; /* enum virDomainPCIConnectFlags */
> char *loadparm;
> +
> + /* PCI devices will only be automatically placed on a PCI bus
> + * that shares the same isolation group */
> + int isolationGroup;
> +
> + /* Usually, PCI buses will take on the same isolation group
> + * as the first device that is plugged into them, but in some
> + * cases we might want to prevent that from happening by
> + * locking the isolation group */
> + bool isolationGroupLocked;
> };
It's vague on purpose :)
All I'm describing there is the interface from the generic
PCI address allocation code's point of view: the fact that
the QEMU driver derives isolation groups from IOMMU groups
is just an implementation detail and as such should not be
mentioned at all.
> > We're never converting back and forth between the two, which
> > I agree would end up in misery at some point down the line;
> > we just set the isolation group once per device and then just
> > perform comparison between isolation groups from there on.
>
> I'd suggest you create a helper to assign those then (be it from IOMMU
> group or something else), so there's at least a single point that can be
> referenced in the future and which will explain this reasoning.
Good idea, I'll do that!
> Also adding a note that 0 means the device is not isolated would make
> sense in the structure above.
Well, devices *always* get isolated: it's just that all
guests except for pSeries only ever have a single isolation
group ;)
--
Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Ján Tomko -
Peter Krempa