Re: [libvirt] [PATCH 02/13] virsh: Split cmds for domain monitoring from virsh.c
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).
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.
Everything else is clean copy-paste, looks good, so ACK with at least
the 'sytax-check' problem fixed.
Martin