On Mon, Nov 26, 2012 at 08:33:23AM -0600, Anthony Liguori wrote:
> fred.kon...@greensocs.com writes:
>
> > From: KONRAD Frederic <fred.kon...@greensocs.com>
> >
> > 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 <fred.kon...@greensocs.com>
> > ---
> > 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: i...@greensocs.com
> > + *
> > + * Developed by :
> > + * Frederic Konrad <fred.kon...@greensocs.com>
> > + *
> > + * 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
