Re: [Qemu-devel] [RFC PATCH v2 1/3] virtio-bus : Introduce VirtioBus.

[Stefan Hajnoczi]

On Mon, Nov 26, 2012 at 08:33:23AM -0600, Anthony Liguori wrote:
> address@hidden writes:
> 
> > From: KONRAD Frederic <address@hidden>
> >
> > This patch create a new VirtioBus, which can be added to Virtio transports 
> > like
> > virtio-pci, virtio-mmio,...
> >
> > One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
> > patch.
> >
> > The VirtioBus shares through a VirtioBusInfo structure :
> >
> >     * two callbacks with the transport : init_cb and exit_cb, which must be
> >       called by the VirtIODevice, after the initialization and before the
> >       destruction, to put the right PCI IDs and/or stop the event fd.
> >
> >     * a VirtIOBindings structure.
> >
> > Signed-off-by: KONRAD Frederic <address@hidden>
> > ---
> >  hw/Makefile.objs |   1 +
> >  hw/virtio-bus.c  | 148 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/virtio-bus.h  |  58 ++++++++++++++++++++++
> >  3 files changed, 207 insertions(+)
> >  create mode 100644 hw/virtio-bus.c
> >  create mode 100644 hw/virtio-bus.h
> >
> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> > index ea46f81..bd14d1b 100644
> > --- a/hw/Makefile.objs
> > +++ b/hw/Makefile.objs
> > @@ -2,6 +2,7 @@ common-obj-y = usb/ ide/
> >  common-obj-y += loader.o
> >  common-obj-$(CONFIG_VIRTIO) += virtio-console.o
> >  common-obj-$(CONFIG_VIRTIO) += virtio-rng.o
> > +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
> >  common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> >  common-obj-y += fw_cfg.o
> >  common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
> > diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
> > new file mode 100644
> > index 0000000..991b6f5
> > --- /dev/null
> > +++ b/hw/virtio-bus.c
> > @@ -0,0 +1,148 @@
> > +/*
> > + * VirtioBus
> > + *
> > + *  Copyright (C) 2012 : GreenSocs Ltd
> > + *      http://www.greensocs.com/ , email: address@hidden
> > + *
> > + *  Developed by :
> > + *  Frederic Konrad   <address@hidden>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +
> > +#include "hw.h"
> > +#include "qemu-error.h"
> > +#include "qdev.h"
> > +#include "virtio-bus.h"
> > +#include "virtio.h"
> > +
> > +#define DEBUG_VIRTIO_BUS 1
> > +
> > +#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) {                        \
> > +                            printf("virtio_bus: " fmt , ## __VA_ARGS__); \
> > +                          }
> 
> #ifdef DEBUG_VIRTIO_BUS
> #define DPRINTF(fmt, ...) ...
> #else
> #define DPRINTF(fmt, ...) do { } while (0)
> #endif
> 
> You're leaving a dangling if clause which can do very strange things if
> used before an else statement.

This should be:

#define DEBUG_VIRTIO_BUS 1

#define DPRINTF(fmt, ...) \
    do { \
        if (DEBUG_VIRTIO_BUS) { \
            printf("virtio_bus: " fmt , ## __VA_ARGS__); \
        } \
    } while (0)

This way there's no dangling else statement problem.  But it also
ensures the we always compile the debug print, thereby avoiding bitrot
you get from #ifdef ... #else ... #endif.

Stefan