Re: [PATCH] Vmchannel PCI device.

Gleb Natapov
2008-12-14T19:43:19Z

On Sun, Dec 14, 2008 at 01:24:01PM -0600, Anthony Liguori wrote:
> 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 :-)
>
I change the comment because I also changes the code it describes.
Previously it registered only block device, now it registers balloon
and vmchannel.

>> + }
>> }
>> 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.
>
Will check :)

>> + */
>> +
>> +#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.
>
OK.

>> 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.
>
OK.

>> - 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.
>
optarg is const and I change the string during parsing (call strsep on it).

--
Gleb.