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.