On 05/20/2011 03:36 AM, Daniel P. Berrange wrote:
> On Fri, May 20, 2011 at 03:05:28PM +0530, Supriya Kannery wrote:
>> Change log level order so that messages at all other levels get logged
>> for "DEBUG" level. Replace magic numbers with corresponding VSH_ERR_xx.
>>
>> } vshErrorLevel;
>
> NACK I don't think this bit is a good idea. In fact I think it
> should be changed to match the log levels in src/util/logging.h
> exactly, so we have the same behaviour for virsh and the rest
> of libvirt.
>
>> @@ -2146,7 +2146,8 @@ cmdDominfo(vshControl *ctl, const vshCmd
>>
>> /* Check and display whether the domain is persistent or not */
>> persistent = virDomainIsPersistent(dom);
>> - vshDebug(ctl, 5, "Domain persistent flag value: %d\n", persistent);
>> + vshDebug(ctl, VSH_ERR_DEBUG, "Domain persistent flag value: %d\n",
>> + persistent);
>> if (persistent < 0)
>
> ACK to all these changes though, hardcoding the numbers instead of using
> the enums was clearly bad.
Unfortunately, I tried to apply this patch in order to pick out just the
magic number removals, but your mailer mungled it beyond the point that
git could understand it. Can you fix the issues we've identified,
including splitting this patch into two pieces (one with just the magic
number removal, another with my suggestion of factoring the log level
comparison into a helper function, and then just tweaking the helper
function and command line -d parsing to nicely map log levels to the
same values as for libvirtd)?
Also, since your mailer seems to be conspiring against us, have you
tried 'git send-email'? Or attaching rather than sending inline?
Practice sending mail to yourself, and running that file through 'git
am' to see what sort of problems we might have applying your patch. Or,
you could post a link to a public repository (github or repo.or.cz are
both decent choices) with your commits pre-applied, to bypass your
mailer idiosyncrasies.
--
Eric Blake eblake redhat com +1-801-349-2682
Libvirt virtualization library http://libvirt.org