Re: [libvirt] [PATCHv2 1/3] xen_vm: convert to typesafe virConf accessors

On Sat, May 26, 2018 at 11:00:25PM +0200, Fabiano Fidêncio wrote: > > From: Fabiano Fidêncio <fidencio(a)redhat.com&gt; > > Signed-off-by: Fabiano Fidêncio <fabiano(a)fidencio.org&gt; The S-o-B address should match the one of the author.

> --- > src/xenconfig/xen_xm.c | 268 > ++++++++++++++++++++++++------------------------- > 1 file changed, 132 insertions(+), 136 deletions(-) > > diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c > index 4becb40b4c..fc88ac8238 100644 > --- a/src/xenconfig/xen_xm.c > +++ b/src/xenconfig/xen_xm.c > @@ -112,172 +112,168 @@ xenParseXMDisk(virConfPtr conf, virDomainDefPtr > def) > { > virDomainDiskDefPtr disk = NULL; > int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; > - virConfValuePtr list = virConfGetValue(conf, "disk"); > + char **disks = NULL, **entries; > > - if (list && list->type == VIR_CONF_LIST) { > - list = list->list; Adding else list = NULL; Would let you move the while below outside of the if body and separate the whitespace changes from the virConfGetValue -> StringList change. Or, even better, the whole per-disk logic can be moved to a separate function.

> - while (list) { > - char *head; > - char *offset; > - char *tmp; > - const char *src; > + if (virConfGetValueStringList(conf, "disk", false, &disks) < 0) > + goto cleanup; > > - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) > - goto skipdisk; > + for (entries = disks; *entries; entries++) { > + char *head = *entries; > + char *offset; > + char *tmp; > + const char *src; > > - head = list->str; > - if (!(disk = virDomainDiskDefNew(NULL))) > - return -1; > + if (!(disk = virDomainDiskDefNew(NULL))) > + return -1; > + > + /* > + * Disks have 3 components, SOURCE,DEST-DEVICE,MODE > + * eg, phy:/dev/HostVG/XenGuest1,xvda,w > + * The SOURCE is usually prefixed with a driver type, > + * and optionally driver sub-type > + * The DEST-DEVICE is optionally post-fixed with disk type > + */ > + > + /* Extract the source file path*/ > + if (!(offset = strchr(head, ','))) > + goto skipdisk; Using a separate function would also get rid of this unusual label.

> + > + if (offset == head) { > + /* No source file given, eg CDROM with no media */ > + ignore_value(virDomainDiskSetSource(disk, NULL)); > + } else { > + if (VIR_STRNDUP(tmp, head, offset - head) < 0) > + goto cleanup; > + > + if (virDomainDiskSetSource(disk, tmp) < 0) { > + VIR_FREE(tmp); > + goto cleanup; > + } > + VIR_FREE(tmp); > + } > [...] > - skipdisk: > - list = list->next; > - virDomainDiskDefFree(disk); > - disk = NULL; > - } > + skipdisk: > + virDomainDiskDefFree(disk); > + disk = NULL; > } > > return 0; > > cleanup: > + virStringListFree(disks); The list needs to be freed even in the success path.

(NB: this usage of 'cleanup' label is not customary - for code paths returning an error, we use 'error'. 'cleanup' is meant for code shared between the success and error paths)

Jano