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

2018-05-07 Thread Lin Ma 



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
---
  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 = NULL;
  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", ) < 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 = 
-data[i].timestamp = timestamp;
-data[i].cb = [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 = 
-data[0].timestamp = timestamp;
-data[0].cb = [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 = 
+data[i].timestamp = timestamp;
+data[i].cb = [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

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

2018-05-04 Thread Peter Krempa 
On Fri, May 04, 2018 at 17:25:30 +0800, Lin Ma wrote:
> Signed-off-by: Lin Ma 
> ---
>  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 = NULL;
>  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", ) < 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

> +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. Also it would be preferrable just to
use one array for the case when events are enumerated and when all are
used ... 

> +}
> +if (VIR_INSERT_ELEMENT(eventIdxList,

Use VIR_APPEND_ELEMENT instead.

> +   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 = 
> -data[i].timestamp = timestamp;
> -data[i].cb = [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 = 
> -data[0].timestamp = timestamp;
> -data[0].cb = [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 = 
> +data[i].timestamp = timestamp;
> +data[i].cb = [all ? i : eventIdxList[i]];

No ternaries, please use the same array, just fill it differently.

> +data[i].id = -1;

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

>  }
>  if (vshCommandOptTimeoutToMs(ctl, cmd, ) < 0)
>  goto cleanup;


signature.asc

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

2018-05-04 Thread Lin Ma 
Signed-off-by: Lin Ma 
---
 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 = NULL;
 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", ) < 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;
+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;
+}
+if (VIR_INSERT_ELEMENT(eventIdxList,
+   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 = 
-data[i].timestamp = timestamp;
-data[i].cb = [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 = 
-data[0].timestamp = timestamp;
-data[0].cb = [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 = 
+data[i].timestamp = timestamp;
+data[i].cb = [all ? i : eventIdxList[i]];
+data[i].id = -1;
 }
 if (vshCommandOptTimeoutToMs(ctl, cmd, ) < 0)
 goto cleanup;
@@ -13441,9 +13442,9 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
 if (vshEventStart(ctl, timeout) < 0)
 goto cleanup;
 
-for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : 1); i++) {
+for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : nevents - 1); i++) {
 if ((data[i].id = virConnectDomainEventRegisterAny(priv->conn, dom,
-   all ? i : event,
+   all ? i : 
eventIdxList[i],
data[i].cb->cb,
[i],