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

Osier Yang jyang at redhat.com
Wed Jul 25 14:08:09 UTC 2012


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




More information about the libvir-list mailing list