Re: [PATCH] Vmchannel PCI device.

[Anthony Liguori <anthony@codemonkey.ws>]

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