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).
I obviously know this amount of diff would generate a noteworthy comment
and it's fine. Still I considered how I've seen this type of processing
done for other modules and I liked the format better where the @def was
being used to fill things in and the @ret was stolen to be returned. To
me that just looks cleaner.
I can like in other instances create a pre-patch that will only make the
def/ret type change and then the magic of VIR_AUTOPTR in the subsequent
patch. But like VIR_STEAL_PTR in general while generating the patches it
felt like I'd end up with a very large series. I was between a rock and
a hard place, so I took the easy way out. Tough to undo years of goto's
and error path additions. While doing this I started to realize these
types of changes probably don't fall into a first time patch for new
users! There needs to be a fair amount of "finesse" to properly order
things.
Tks -
John
>
> 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
