[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: minor cleanups



On 02/03/2014 10:52 AM, Christophe Fergeau wrote:
> On Fri, Jan 31, 2014 at 05:08:32PM +0100, Martin Kletzander wrote:
>> Some cleanups around serial chardevs and miscellaneous things I've
>> found inconsistent or not very clean.
> 
> A few comments below, I'd tend to split at least the bigger bits in
> separate patches to avoid a bunch of unrelated changes in a single commit
> 
> Christophe

I agree - please break up into smaller pieces, since it makes
backporting easier (not all the problems being cleaned here were
introduced in the same release).

>>
>> -    switch (src->type) {
>> +    switch ((enum virDomainChrType)src->type) {
> 
> Not sure this one improves things, the type of the enum is in the
> namespacing of the values anyway (VIR_DOMAIN_CHR_TYPE_*), and it's only
> done for a minority of the switch() calls in domain_conf.c

Adding the explicit cast DOES help - it makes the compiler flag a
warning (which with -Werror is fatal) if the user adds an enum value but
forgets to update this switch statement.


>>      case VIR_DOMAIN_CHR_TYPE_STDIO:
>>      case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
>> +    case VIR_DOMAIN_CHR_TYPE_LAST:
>>          /* nada */
>>          return true;
>>      }
>>
>> -    /* This should happen only on new,
>> -     * yet unhandled type */
>> +    /* Even though gcc is able to detect that all possible values are
>> +     * handled in the switch above, it is not capable of realizing
>> +     * there isn't any possibility of escaping that switch without
>> +     * return.  So for the sake of clean compilation, there has to be
>> +     * a return here */
>>
>> +    /* coverity[dead_error_begin] */
>>      return false;


Rather than a big long hairy comment here, I'd go with the simpler:

...
case VIR_DOMAIN_CHR_TYPE_LAST:
    /* nada */
    break;
}

return true;

so that at least one case falls out of the switch statement, and then
the ending return is no longer dead code.


>>
>>  #define NET_MODEL_CHARS \
>> -    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ091234567890_-"
>> +    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-"

Wonder what happened to cause the duplication in the original?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]