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

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



On Mon, Feb 03, 2014 at 11:10:10AM -0700, Eric Blake wrote:
> 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).
>

OK, will do, I just missed your mail when replying to Christophe.

> >>
> >> -    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?
>

I can only guess that it might have been a change from 0-9, but I
can't find anyone better than you to ask ;)

v2 coming up

Martin

Attachment: signature.asc
Description: Digital signature


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