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

On 2/7/19 8:06 AM, Erik Skultety wrote: > 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. So the point wasn't to get rid of the label, the point was to be consistent. A "side reference" to this is the LGTM static analysis website Daniel pointed out last week in some patches. One of the "alerts" they post is use of "goto backwards". It was in a slightly different usage model, but multiple labels in the code has gotten us in trouble before especially when code is refactored or a couple lines moved where a goto cleanup should now be a goto error or goto endjob (more likely).