Re: [PATCH] Vmchannel PCI device.

Anthony Liguori
2008-12-14T19:24:06Z

Gleb Natapov wrote:
> diff --git a/hw/pc.c b/hw/pc.c
> index 73dd8bc..57e3b1d 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1095,7 +1095,7 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
> }
> }
>
> - /* Add virtio block devices */
> + /* Add virtio devices */
>

Please don't make comments less specific. We probably want to go in the
opposite direction :-)

> if (pci_enabled) {
> int index;
> int unit_id = 0;
> @@ -1104,11 +1104,9 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
> virtio_blk_init(pci_bus, drives_table[index].bdrv);
> unit_id++;
> }
> - }
> -
> - /* Add virtio balloon device */
> - if (pci_enabled)
> virtio_balloon_init(pci_bus);
> + virtio_vmchannel_init(pci_bus);
>

You're tab damaged.

> + }
> }
>
> static void pc_init_pci(ram_addr_t ram_size, int vga_ram_size,
> diff --git a/hw/virtio-vmchannel.c b/hw/virtio-vmchannel.c
> new file mode 100644
> index 0000000..1f5e274
> --- /dev/null
> +++ b/hw/virtio-vmchannel.c
> @@ -0,0 +1,283 @@
> +/*
> + * Virtio VMChannel Device
> + *
> + * Copyright RedHat, inc. 2008
> + *
> + * Authors:
> + * Gleb Natapov <gleb@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
>

Did you intend for GPLv2 or GPLv2+? There's no requirement either way
but sometimes people just copy/paste these things.

> + */
> +
> +#include "qemu-common.h"
> +#include "sysemu.h"
> +#include "virtio.h"
> +#include "pc.h"
> +#include "qemu-char.h"
> +#include "virtio-vmchannel.h"
> +
> +//#define DEBUG_VMCHANNEL
> +
> +#ifdef DEBUG_VMCHANNEL
> +#define VMCHANNEL_DPRINTF(fmt, args...) \
> + do { printf("VMCHANNEL: " fmt , ##args); } while (0)
> +#else
> +#define VMCHANNEL_DPRINTF(fmt, args...)
> +#endif
>

I very much like just naming these things dprintf() but this is not a
requirement. Please do use C99 style though instead of GCC-ism.

> diff --git a/sysemu.h b/sysemu.h
> index 94cffaf..54d9c83 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -157,6 +157,10 @@ extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>
> #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
>
> +#define MAX_VMCHANNEL_DEVICES 4
> +void virtio_vmchannel_init(PCIBus *bus);
> +void vmchannel_init(CharDriverState *hd, const char *name);
>

This should be in virtio-vmchannel.h.

> - case QEMU_OPTION_loadvm:
> + case QEMU_OPTION_vmchannel:
> + if (vmchannel_device_index >= MAX_VMCHANNEL_DEVICES) {
> + fprintf(stderr, "qemu: too many vmchannel devices\n");
> + exit(1);
> + }
> + vmchannel_devices[vmchannel_device_index++] = strdup(optarg);
>

No need to strdup(). optarg is good for the duration of execution.

I've only done a light review but things mostly look good. I'd like to
wait a bit to see what the reaction is on netdev before applying.

Regards,

Anthony Liguori