Re: [PATCH] Vmchannel PCI device.

[Gleb Natapov <gleb@redhat.com>]

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.