Re: [PATCH] virsh: simply printing of typed parameters

[Eric Blake <eblake@xxxxxxxxxx>]

On 12/19/2011 04:57 PM, Peter Krempa wrote:
> Dňa 20.12.2011 0:08, Eric Blake  wrote / napísal(a):
>> No need to repeat code for formatting typed parameters.
>>
>> * tools/virsh.c (vshGetTypedParamValue): Support strings.
>> (cmdSchedinfo, cmdBlkiotune, cmdMemtune, cmdBlkdeviotune): Use
>> it for less code.
>> ---
>>   tools/virsh.c |  134
>> +++++++++------------------------------------------------
>>   1 files changed, 21 insertions(+), 113 deletions(-)
>>
>> +            char *str = vshGetTypedParamValue(ctl,&params[i]);
> ( in all other instances )
> vshGetTypedParamValue uses virAsprintf internaly, so there's a
> possiblity that it will return NULL as the parameter if an error happens.

Why not go one step further - since the only error is OOM, and we
already use vshStrdup as an instant exit on OOM, I can just make
vshGetTypedParamValue guarantee a non-NULL return (exit on OOM).

> 
> Please squash in this fix along with checking for return value in
> instances you added. This fixes a function that I tampered with
> previously. The code did not skip to the end, if vshGetTypedParamValue
> returned NULL. Thanks.
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index a3ec7e9..fa66579 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -1187,7 +1187,7 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd)
>                  continue;
> 
>              if (!(value = vshGetTypedParamValue(ctl, par)))
> -                continue;
> +                goto cleanup;

By guaranteeing non-NULL, we don't even need the if.

> 
> ACK with the check for return value added. Nice reduction of lines
> though :)

Pushed with even more lines shaved, as well as adding a missing 'break':

diff --git i/tools/virsh.c w/tools/virsh.c
index a3ec7e9..4f3c9f8 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -387,7 +387,8 @@ static bool vshConnectionUsability(vshControl *ctl,
virConnectPtr conn);
 static virTypedParameterPtr vshFindTypedParamByName(const char *name,

virTypedParameterPtr list,
                                                     int count);
-static char *vshGetTypedParamValue(vshControl *ctl,
virTypedParameterPtr item);
+static char *vshGetTypedParamValue(vshControl *ctl,
virTypedParameterPtr item)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

 static char *editWriteToTempFile (vshControl *ctl, const char *doc);
 static int   editFile (vshControl *ctl, const char *filename);
@@ -1186,8 +1187,7 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd)
                                                 nparams)))
                 continue;

-            if (!(value = vshGetTypedParamValue(ctl, par)))
-                continue;
+            value = vshGetTypedParamValue(ctl, par);

             /* to print other not supported fields, mark the already
printed */
             par->field[0] = '\0'; /* set the name to empty string */
@@ -1214,9 +1214,7 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd)
             if (!*params[i].field)
                 continue;

-            if (!(value = vshGetTypedParamValue(ctl, params+i)))
-                continue;
-
+            value = vshGetTypedParamValue(ctl, params+i);
             vshPrint(ctl, "%s %s %s\n", device, params[i].field, value);
             VIR_FREE(value);
         }
@@ -16956,15 +16954,14 @@ vshDomainStateReasonToString(int state, int
reason)
     return N_("unknown");
 }

+/* Return a non-NULL string representation of a typed parameter; exit
+ * if we are out of memory.  */
 static char *
 vshGetTypedParamValue(vshControl *ctl, virTypedParameterPtr item)
 {
     int ret = 0;
     char *str = NULL;

-    if (!ctl || !item)
-        return NULL;
-
     switch(item->type) {
     case VIR_TYPED_PARAM_INT:
         ret = virAsprintf(&str, "%d", item->value.i);
@@ -16991,15 +16988,17 @@ vshGetTypedParamValue(vshControl *ctl,
virTypedParameterPtr item)
         break;

     case VIR_TYPED_PARAM_STRING:
-        str = vshStrdup (ctl, item->value.s);
-        ret = str ? 0 : -1;
+        str = vshStrdup(ctl, item->value.s);
+        break;

     default:
         vshError(ctl, _("unimplemented parameter type %d"), item->type);
     }

-    if (ret < 0)
+    if (ret < 0) {
         vshError(ctl, "%s", _("Out of memory"));
+        exit(EXIT_FAILURE);
+    }
     return str;
 }

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list