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