Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize()

[Cao jin <caoj.fnst@cn.fujitsu.com>]

Hi Stefano,
     first of all, thanks for your quick response:)

On 12/23/2015 08:03 PM, Stefano Stabellini wrote:
> On Wed, 23 Dec 2015, Cao jin wrote:
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>
>> Since the callchain is pretty deep & error path is very much too, so I made the
>> patch based on the principal: catch/report the most necessary error msg with
>> smallest modification.(So you can see I don`t change some functions to void,
>> despite they coule be)
>
> Thanks Cao.
>
> For consistency with the other functions, I think it would be better to
> change all functions to return void or none.
>

Ok, I`ll select one style may with the smallest modification;)

> Also it might be nice to split the patch in a series.
>

Yup, and the patches should be independent from each other?

> The patch as is fails to build:
>
> qemu/hw/xen/xen_pt_config_init.c: In function ‘xen_pt_config_init’:
> qemu/hw/xen/xen_pt_config_init.c:2061:42: error: ‘rc’ may be used uninitialized in this func
>

really weird...last patch you remind me that it cannot compile, make me 
find that my computer didn`t install xen-devel package, then I installed 
it right away. But this time, it really can compile on my 
computer....anyway, I will check it out later.

>
>>   hw/xen/xen-host-pci-device.c | 79 +++++++++++++++++++++++++++-----------------
>>   hw/xen/xen-host-pci-device.h |  5 +--
>>   hw/xen/xen_pt.c              | 67 +++++++++++++++++++------------------
>>   hw/xen/xen_pt.h              |  5 +--
>>   hw/xen/xen_pt_config_init.c  | 47 +++++++++++++-------------
>>   hw/xen/xen_pt_graphics.c     |  6 ++--
>>   6 files changed, 118 insertions(+), 91 deletions(-)
>>
>> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
>> index 7d8a023..1ab6d97 100644
>> --- a/hw/xen/xen-host-pci-device.c
>> +++ b/hw/xen/xen-host-pci-device.c
>> @@ -43,13 +43,14 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>>           /* The output is truncated, or some other error was encountered */
>>           return -ENODEV;
>>       }
>> +
>>       return 0;
>>   }
>
> I would prefer to keep stylistic changes separate, especially the ones
> in functions which would be otherwise left unmodified. Maybe you could
> move them to a separate patch?
>

I can do that.

>
[...]
>> +
>>       if (i != PCI_NUM_REGIONS) {
>>           /* Invalid format or input to short */
>> -        rc = -ENODEV;
>> +        error_setg(errp, "Invalid format or input to short");
>
>                                                       ^too short

How about printing all the string in buf? like:
"Invalid format or input to short: %s", buf

for all the other comments below: will fix them up:)
>
>>       }
>>
>>   out:
>>       close(fd);
>> -    return rc;
>>   }
>>
[...]
>

-- 
Yours Sincerely,

Cao Jin