From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LBwZT-0000oN-0m for qemu-devel@nongnu.org; Sun, 14 Dec 2008 14:24:07 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LBwZS-0000o4-6D for qemu-devel@nongnu.org; Sun, 14 Dec 2008 14:24:06 -0500 Received: from [199.232.76.173] (port=38213 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LBwZS-0000nx-0o for qemu-devel@nongnu.org; Sun, 14 Dec 2008 14:24:06 -0500 Received: from yx-out-1718.google.com ([74.125.44.154]:6901) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LBwZR-0008S8-LC for qemu-devel@nongnu.org; Sun, 14 Dec 2008 14:24:05 -0500 Received: by yx-out-1718.google.com with SMTP id 3so1082485yxi.82 for ; Sun, 14 Dec 2008 11:24:05 -0800 (PST) Message-ID: <49455D51.5080004@codemonkey.ws> Date: Sun, 14 Dec 2008 13:24:01 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <20081214115027.4028.56164.stgit@dhcp-1-237.tlv.redhat.com> In-Reply-To: <20081214115027.4028.56164.stgit@dhcp-1-237.tlv.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] Vmchannel PCI device. Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gleb Natapov Cc: qemu-devel@nongnu.org, 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 > + * > + * 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