From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anthony Liguori <anthony@codemonkey.ws>
Subject: Re: [PATCH] Vmchannel PCI device.
Date: Sun, 14 Dec 2008 13:24:01 -0600
Message-ID: <49455D51.5080004@codemonkey.ws>
References: <20081214115027.4028.56164.stgit@dhcp-1-237.tlv.redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org
To: Gleb Natapov <gleb@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from an-out-0708.google.com ([209.85.132.245]:2413 "EHLO
	an-out-0708.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751275AbYLNTYG (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 14 Dec 2008 14:24:06 -0500
Received: by an-out-0708.google.com with SMTP id d40so988742and.1
        for <kvm@vger.kernel.org>; Sun, 14 Dec 2008 11:24:05 -0800 (PST)
In-Reply-To: <20081214115027.4028.56164.stgit@dhcp-1-237.tlv.redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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