Re: [libvirt] [PATCH 03/15] conf: Introduce VIR_DEFINE_AUTOPTR_FUNC for virStorageVolDef

On Wed, Feb 06, 2019 at 08:41:35AM -0500, John Ferlan wrote: > Let's make use of the auto __cleanup capabilities cleaning up any > now unnecessary goto paths. For virStorageVolDefParseXML use the > @def/@ret similarly as other methods. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- ... > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index 6099c64b26..83ca379217 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -1168,7 +1168,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, > xmlXPathContextPtr ctxt, > unsigned int flags) > { > - virStorageVolDefPtr ret; > + VIR_AUTOPTR(virStorageVolDef) def = NULL; > + virStorageVolDefPtr ret = NULL; > virStorageVolOptionsPtr options; > char *type = NULL; > char *allocation = NULL; > @@ -1187,132 +1188,132 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, > if (options == NULL) > return NULL; > > - if (VIR_ALLOC(ret) < 0) > + if (VIR_ALLOC(def) < 0) > return NULL; > > - ret->target.type = VIR_STORAGE_TYPE_FILE; > + def->target.type = VIR_STORAGE_TYPE_FILE; > > - ret->name = virXPathString("string(./name)", ctxt); > - if (ret->name == NULL) { > + def->name = virXPathString("string(./name)", ctxt); > + if (def->name == NULL) { > virReportError(VIR_ERR_XML_ERROR, "%s", > _("missing volume name element")); > - goto error; > + goto cleanup; > } > > /* Normally generated by pool refresh, but useful for unit tests */ > - ret->key = virXPathString("string(./key)", ctxt); > + def->key = virXPathString("string(./key)", ctxt); > > /* Technically overridden by pool refresh, but useful for unit tests */ > type = virXPathString("string(./@type)", ctxt); > if (type) { > - if ((ret->type = virStorageVolTypeFromString(type)) < 0) { > + if ((def->type = virStorageVolTypeFromString(type)) < 0) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("unknown volume type '%s'"), type); > - goto error; > + goto cleanup; > } > } > > if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { > - if (VIR_ALLOC(ret->target.backingStore) < 0) > - goto error; > + if (VIR_ALLOC(def->target.backingStore) < 0) > + goto cleanup; > > - ret->target.backingStore->type = VIR_STORAGE_TYPE_FILE; > + def->target.backingStore->type = VIR_STORAGE_TYPE_FILE; > > - ret->target.backingStore->path = backingStore; > + def->target.backingStore->path = backingStore; > backingStore = NULL; > > if (options->formatFromString) { > char *format = virXPathString("string(./backingStore/format/@type)", ctxt); > if (format == NULL) > - ret->target.backingStore->format = options->defaultFormat; > + def->target.backingStore->format = options->defaultFormat; > else > - ret->target.backingStore->format = (options->formatFromString)(format); > + def->target.backingStore->format = (options->formatFromString)(format); > > - if (ret->target.backingStore->format < 0) { > + if (def->target.backingStore->format < 0) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("unknown volume format type %s"), format); > VIR_FREE(format); > - goto error; > + goto cleanup; > } > VIR_FREE(format); > } > > - if (VIR_ALLOC(ret->target.backingStore->perms) < 0) > - goto error; > - if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms, > + if (VIR_ALLOC(def->target.backingStore->perms) < 0) > + goto cleanup; > + if (virStorageDefParsePerms(ctxt, def->target.backingStore->perms, > "./backingStore/permissions") < 0) > - goto error; > + goto cleanup; > } > > capacity = virXPathString("string(./capacity)", ctxt); > unit = virXPathString("string(./capacity/@unit)", ctxt); > if (capacity) { > - if (virStorageSize(unit, capacity, &ret->target.capacity) < 0) > - goto error; > + if (virStorageSize(unit, capacity, &def->target.capacity) < 0) > + goto cleanup; > } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY) && > !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && > - virStorageSourceHasBacking(&ret->target))) { > + virStorageSourceHasBacking(&def->target))) { > virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element")); > - goto error; > + goto cleanup; > } > VIR_FREE(unit); > > allocation = virXPathString("string(./allocation)", ctxt); > if (allocation) { > unit = virXPathString("string(./allocation/@unit)", ctxt); > - if (virStorageSize(unit, allocation, &ret->target.allocation) < 0) > - goto error; > - ret->target.has_allocation = true; > + if (virStorageSize(unit, allocation, &def->target.allocation) < 0) > + goto cleanup; > + def->target.has_allocation = true; > } else { > - ret->target.allocation = ret->target.capacity; > + def->target.allocation = def->target.capacity; > } > > - ret->target.path = virXPathString("string(./target/path)", ctxt); > + def->target.path = virXPathString("string(./target/path)", ctxt); > if (options->formatFromString) { > char *format = virXPathString("string(./target/format/@type)", ctxt); > if (format == NULL) > - ret->target.format = options->defaultFormat; > + def->target.format = options->defaultFormat; > else > - ret->target.format = (options->formatFromString)(format); > + def->target.format = (options->formatFromString)(format); > > - if (ret->target.format < 0) { > + if (def->target.format < 0) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("unknown volume format type %s"), format); > VIR_FREE(format); > - goto error; > + goto cleanup; > } > VIR_FREE(format); > } > > - if (VIR_ALLOC(ret->target.perms) < 0) > - goto error; > - if (virStorageDefParsePerms(ctxt, ret->target.perms, > + if (VIR_ALLOC(def->target.perms) < 0) > + goto cleanup; > + if (virStorageDefParsePerms(ctxt, def->target.perms, > "./target/permissions") < 0) > - goto error; > + goto cleanup; > > node = virXPathNode("./target/encryption", ctxt); > if (node != NULL) { > - ret->target.encryption = virStorageEncryptionParseNode(node, ctxt); > - if (ret->target.encryption == NULL) > - goto error; > + def->target.encryption = virStorageEncryptionParseNode(node, ctxt); > + if (def->target.encryption == NULL) > + goto cleanup; > } > > - ret->target.compat = virXPathString("string(./target/compat)", ctxt); > - if (virStorageFileCheckCompat(ret->target.compat) < 0) > - goto error; > + def->target.compat = virXPathString("string(./target/compat)", ctxt); > + if (virStorageFileCheckCompat(def->target.compat) < 0) > + goto cleanup; > > if (virXPathNode("./target/nocow", ctxt)) > - ret->target.nocow = true; > + def->target.nocow = true; > > if (virXPathNode("./target/features", ctxt)) { > if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0) > - goto error; > + goto cleanup; > > - if (!ret->target.compat && VIR_STRDUP(ret->target.compat, "1.1") < 0) > - goto error; > + if (!def->target.compat && VIR_STRDUP(def->target.compat, "1.1") < 0) > + goto cleanup; > > - if (!(ret->target.features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) > - goto error; > + if (!(def->target.features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) > + goto cleanup; > > for (i = 0; i < n; i++) { > int f = virStorageFileFeatureTypeFromString((const char*)nodes[i]->name); > @@ -1320,13 +1321,15 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, > if (f < 0) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported feature %s"), > (const char*)nodes[i]->name); > - goto error; > + goto cleanup; > } > - ignore_value(virBitmapSetBit(ret->target.features, f)); > + ignore_value(virBitmapSetBit(def->target.features, f)); > } > VIR_FREE(nodes); > } > > + VIR_STEAL_PTR(ret, def); > + > cleanup: > VIR_FREE(nodes); > VIR_FREE(allocation); > @@ -1335,11 +1338,6 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, > VIR_FREE(type); > VIR_FREE(backingStore); > return ret; > - > - error: > - virStorageVolDefFree(ret); > - ret = NULL; I know I'm going to contradict myself since I didn't say anything in one of the previous patches where you did the same thing, however, this really caught my eye - the diff is just too large (essentially just noise in the history) for the benefit of getting rid of the last 3 lines, in this particular case, I'd leave it out completely. Alternatively, you could have a preceding patch where you'd add a new variable, perform the necessary replacements including VIR_STEAL_PTR and then in this patch convert to AUTOPTR only, but as I said to Sukrit in some of his patches, sometimes the replacement just wasn't worth the resulting diff just to get rid of a label, that's my opinion.

To the rest of the changes: Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;