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@xxxxxxxxxx>
> ---
...
> 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@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
