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

Re: [libvirt] [PATCH 02/13] virsh: Split cmds for domain monitoring from virsh.c



On 2012年07月25日 21:20, Martin Kletzander wrote:
On 07/24/2012 11:18 AM, Osier Yang wrote:
This splits commands commands to monitor domain status into
virsh-domain-monitor.c. The helpers not for common use are moved too.
Standard copyright is added.

* tools/virsh.c:
   Remove commands for domain monitoring group and a few helpers (
   vshDomainIOErrorToString, vshGetDomainDescription,
   vshDomainControlStateToString, vshDomainStateToString) not for
   common use.

* tools/virsh-domain-monitor.c:
   New file, filled with commands of domain monitor group.
---
  tools/virsh-domain-monitor.c | 1685 +++++++++++++++++++++++++++++++++++++
  tools/virsh.c                | 1904 +++---------------------------------------
  2 files changed, 1807 insertions(+), 1782 deletions(-)
  create mode 100644 tools/virsh-domain-monitor.c

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
new file mode 100644
index 0000000..1a61f62
--- /dev/null
+++ b/tools/virsh-domain-monitor.c
@@ -0,0 +1,1685 @@
+/*
+ * virsh-domain.c: Commands to monitor domain status

Wrong filename in the header, should be virsh-domain-monitor.c

+ *
+ * Copyright (C) 2005, 2007-2012 Red Hat, Inc
[...]
+cleanup:
+    VIR_FREE(domxml);
+    xmlXPathFreeContext(ctxt);
+    xmlFreeDoc(doc);
+
+    return desc;
+}

I'd add a empty line here for the sake of beauty.

+static const char *
+vshDomainControlStateToString(int state)
[...]
+static const char *
+vshDomainStateToString(int state)
+{
+    /* Can't use virDomainStateTypeToString, because we want to mark
+ *      * strings for translation.  */

This comment has weirdly shifted asterisk.

+    switch ((virDomainState) state) {
[...]
+
+/*
+ * "dommemstats" command

Should be 'dommemstat'

Also the order of the commands doesn't make much sense to me (it could
be in the same order as the domMonitoringCmds struct).

Actually I tried to sort that. But I gave up quickly for it will take
too much time. And honestly, it's boring.


Apart from the tiny beauty nits (I'd give ACK with that), there is a
problem with 'cfg.mk'. It specifies 'virsh.c' as one of the files with
exception for strcasecmp() and this function gets copied into
'virsh-domain-monitor.c'. If this is done because of this particular
occurrence, the file should be added to
'exclude_file_name_regexp--sc_avoid_strcase' in 'cfg.mk', otherwise make
syntax-check will fail.

Good catch, I did 'syntax-check', but not sure why I missed it.


Everything else is clean copy-paste, looks good, so ACK with at least
the 'sytax-check' problem fixed.


Will fix those nits and the syntax-check failure when pushing (
after the whole set is acked). Thanks for the reviewing.

Regards,
Osier


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