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?