On Mon, Feb 03, 2014 at 06:52:49PM +0100, 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
>
If you want to, I have no problems doing that, I just thought these
are all basically no-op clean-ups anyway.
> Christophe
>
> >
> > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> > ---
> > src/conf/domain_conf.c | 43 ++++++++++++++++++++++++++-----------------
> > src/qemu/qemu_capabilities.c | 4 ++--
> > src/qemu/qemu_command.c | 10 ++++++++--
> > 3 files changed, 36 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 28e24f9..fa1ecb5 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -1559,7 +1559,7 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src,
> > if (tgt->type != src->type)
> > return false;
> >
> > - 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
>
Actually, for now it doesn't. But it might do the job of future
problem catcher if new values are added to that enum and it might be
desirable to add them to this switch (as compiler will find that). In
case they are not, adding them to the no-op branch shows that the
author of such patch probably thought it through (or at least know
that there's a code taking decision based on such value).
I like the idea of listing all the enums and compiler reminding about
places depending on them. Also, listing them (at least for no-op
cases) in the same order that's in the enum makes it faster to skim
through the code someimes in case there's more of them. That
motivated me to fix a least few of these switches.
> > case VIR_DOMAIN_CHR_TYPE_PTY:
> > case VIR_DOMAIN_CHR_TYPE_DEV:
> > case VIR_DOMAIN_CHR_TYPE_FILE:
[...]
> > @@ -6071,6 +6071,12 @@ qemuBuildChrArgStr(virDomainChrSourceDefPtr dev, const char *prefix)
> > dev->data.nix.path,
> > dev->data.nix.listen ? ",server,nowait" : "");
> > break;
> > +
> > + case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
> > + /* spicevmc doesn't have any '-serial' compatible option */
>
> This is misleading as this function is also called for -monitor, and
> -parallel.
>
Oh, that's right. I'll just join it with the below one if omitting
the comment is ok.
Thanks for looking at the patch,
Martin
> > + case VIR_DOMAIN_CHR_TYPE_LAST:
> > + /* coverity[dead_error_begin] */
> > + break;
> > }
> >
> > if (virBufferError(&buf)) {
> > --
> > 1.8.5.3
> >
> > --
> > libvir-list mailing list
> > libvir-list@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list