On Thu, Feb 07, 2019 at 09:26:44AM -0500, John Ferlan wrote:
>
>
> 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@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.
>
> 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).
Even if a static analyser flagged our
cleanup:
return;
error:
goto cleanup;
convention, then, more likely than not, it's a false positive I don't see much
point in fixing just for the sake of satisfying an algorithm. At the same time
though, my only argument is that by having diffs like the one in play here
will need more git blame magic in vim by developers to find what they're
looking for - an argument, which by itself, is a pretty weak one. But if you
split the changes as mentioned already, I guess everyone will be okay with the
change.
Erik
