Re: [PATCH 09/12] virsh: Enable multiple --event flags to 'event' command

[Lin Ma <lma@xxxxxxxx>]

On 05/04/2018 06:44 PM, Peter Krempa wrote:

On Fri, May 04, 2018 at 17:25:30 +0800, Lin Ma wrote:

Signed-off-by: Lin Ma <lma@xxxxxxxx>
---
 tools/virsh-domain.c | 76 +++++++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 37 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 89aefbf86a..b35c9adaaa 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -13347,10 +13347,6 @@ static const vshCmdInfo info_event[] = {
 static const vshCmdOptDef opts_event[] = {
     VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("filter by domain name, id or uuid"),
                                       0),
-    {.name = "event",
-     .type = VSH_OT_STRING,
-     .help = N_("which event type to wait for")
-    },
     {.name = "all",
      .type = VSH_OT_BOOL,
      .help = N_("wait for all events instead of just one type")
@@ -13371,6 +13367,10 @@ static const vshCmdOptDef opts_event[] = {
      .type = VSH_OT_BOOL,
      .help = N_("show timestamp for each printed event")
     },
+    {.name = "event",
+     .type = VSH_OT_ARGV,
+     .help = N_("which event type to wait for")
+    },
     {.name = NULL}
 };
 
@@ -13382,12 +13382,14 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
     int timeout = 0;
     virshDomEventData *data = ""
     size_t i;
-    const char *eventName = NULL;
+    int *eventIdxList = NULL;
+    size_t nevents = 0;
     int event = -1;
     bool all = vshCommandOptBool(cmd, "all");
     bool loop = vshCommandOptBool(cmd, "loop");
     bool timestamp = vshCommandOptBool(cmd, "timestamp");
     int count = 0;
+    const vshCmdOpt *opt = NULL;
     virshControlPtr priv = ctl->privData;
 
     if (vshCommandOptBool(cmd, "list")) {
@@ -13396,15 +13398,25 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
         return true;
     }
 
-    if (vshCommandOptStringReq(ctl, cmd, "event", &eventName) < 0)
-        return false;
-    if (eventName) {
-        for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++)
-            if (STREQ(eventName, vshEventCallbacks[event].name))
-                break;
-        if (event == VIR_DOMAIN_EVENT_ID_LAST) {
-            vshError(ctl, _("unknown event type %s"), eventName);
-            return false;
+    if (vshCommandOptBool(cmd, "event")) {
+        if (VIR_ALLOC_N(eventIdxList, 1) < 0)
+            goto cleanup;

This is not necessary, VIR_APPEND_ELEMENT does that

will remove it.

+        nevents = 1;
+        while ((opt = vshCommandOptArgv(ctl, cmd, opt))) {
+            if (opt->data) {
+                for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++)
+                    if (STREQ(opt->data, vshEventCallbacks[event].name))
+                        break;
+                if (event == VIR_DOMAIN_EVENT_ID_LAST) {
+                    vshError(ctl, _("unknown event type %s"), opt->data);
+                    return false;

This leaks the eventIdxList array. 

ok, will fix it through using 'goto cleanup' instead of 'return false'

Also it would be preferrable just to
use one array for the case when events are enumerated and when all are
used ... 

Do you mean that the code logical should not go '--all' but '--event A' in the case 'virsh event --event A --all'?

+                }
+                if (VIR_INSERT_ELEMENT(eventIdxList,

Use VIR_APPEND_ELEMENT instead.

will do.

+                                       nevents - 1,
+                                       nevents,
+                                       event) < 0)
+                    goto cleanup;
+            }
         }
     } else if (!all) {
         vshError(ctl, "%s",
@@ -13412,26 +13424,15 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
         return false;
     }
 
-    if (all) {
-        if (VIR_ALLOC_N(data, VIR_DOMAIN_EVENT_ID_LAST) < 0)
-            goto cleanup;
-        for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) {
-            data[i].ctl = ctl;
-            data[i].loop = loop;
-            data[i].count = &count;
-            data[i].timestamp = timestamp;
-            data[i].cb = &vshEventCallbacks[i];
-            data[i].id = -1;
-        }
-    } else {
-        if (VIR_ALLOC_N(data, 1) < 0)
-            goto cleanup;
-        data[0].ctl = ctl;
-        data[0].loop = vshCommandOptBool(cmd, "loop");
-        data[0].count = &count;
-        data[0].timestamp = timestamp;
-        data[0].cb = &vshEventCallbacks[event];
-        data[0].id = -1;
+    if (VIR_ALLOC_N(data, all ? VIR_DOMAIN_EVENT_ID_LAST : nevents - 1) < 0)
+        goto cleanup;
+    for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : nevents - 1); i++) {
+        data[i].ctl = ctl;
+        data[i].loop = loop;
+        data[i].count = &count;
+        data[i].timestamp = timestamp;
+        data[i].cb = &vshEventCallbacks[all ? i : eventIdxList[i]];

No ternaries, 

ok

please use the same array, just fill it differently.

do you mean that
whatever in 'all' or 'not all', I should fill 'eventIdxList' differently & use 'eventIdxList' in just one for loop?
if so, that means:
in case '--all': the value of array eventIdxList will be from 0 through VIR_DOMAIN_EVENT_ID_LAST - 1.
in case '--event' or '--event and --all': the value of array eventIdxList will be corresponding event id(s).
not sure what I understand is right.

+        data[i].id = -1;

You can refactor the above to use VIR_APPEND_ELEMENT too so that the
same array can be used.

Sorry, I didn't get the point. which one should be refactored to use VIR_APPEND_ELEMENT as well? 'eventIdxList' for case '--all'?



Thanks,
Lin

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