[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 02/12] test: Adjust cleanup/error paths for nodedev test APIs



On Sat, Jun 03, 2017 at 09:11:52AM -0400, John Ferlan wrote:
>  - Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName
>    an @obj, just return directly.  This then allows the cleanup: label code
>    to not have to check "if (obj)" before calling virNodeDeviceObjUnlock.
>    This also simplifies some exit logic...
>
>  - In testNodeDeviceObjFindByName use an error: label to handle the failure
>    and don't do the ncaps++ within the VIR_STRDUP() source target index.
>    Only increment ncaps after success. Easier on eyes at error label too.
>
>  - In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto
>
> Signed-off-by: John Ferlan <jferlan redhat com>

[..]

>      event = virNodeDeviceEventLifecycleNew("scsi_host12",
> @@ -4534,13 +4533,8 @@ testDestroyVport(testDriverPtr privconn,
>      virNodeDeviceObjFree(obj);
>      obj = NULL;

You can drop ^this then since you don't need to clear it anymore.

Just a minor nit, while I was checking where testDestroyVport gets called from
- testStoragePoolDestroy, there's a spot that could be fixed the same way.
Would you mind squashing this bit in as well?

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 6422ba8fb..a397364c7 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4547,7 +4547,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool)
     virObjectEventPtr event = NULL;

     if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name)))
-        goto cleanup;
+        return -1;

     if (!virStoragePoolObjIsActive(privpool)) {
         virReportError(VIR_ERR_OPERATION_INVALID,



[...]

> @@ -5424,27 +5408,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
>      virNodeDeviceDefPtr def;
>      virNodeDevCapsDefPtr caps;
>      int ncaps = 0;
> -    int ret = -1;
>
>      if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
> -        goto cleanup;
> +        return -1;
>      def = virNodeDeviceObjGetDef(obj);
>
>      for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) {
> -        if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0)
> -            goto cleanup;
> +        if (VIR_STRDUP(names[ncaps],
> +                       virNodeDevCapTypeToString(caps->data.type)) < 0)
> +            goto error;
> +        ncaps++;

How about placing the increment to the iteration_expression segment of the
loop, aka at the end where the usual increment happens before condition
evaluation. Wondering whether there's a general technical term for the syntax
part of the loop between the parentheses.

>      }
> -    ret = ncaps;
>
> - cleanup:
> -    if (obj)
> -        virNodeDeviceObjUnlock(obj);
> -    if (ret == -1) {
> -        --ncaps;
> -        while (--ncaps >= 0)
> -            VIR_FREE(names[ncaps]);

Hmm, this was indeed an interesting bit of code.

> -    }
> -    return ret;
> +    virNodeDeviceObjUnlock(obj);
> +    return ncaps;
> +
> + error:
> +    while (--ncaps >= 0)
> +        VIR_FREE(names[ncaps]);
> +    virNodeDeviceObjUnlock(obj);
> +    return -1;
>  }
>

ACK with adjustments.

Erik


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]