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
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)