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

[Stefano Stabellini]

On Wed, 23 Dec 2015, Cao jin wrote:
> Hi Stefano,
>     first of all, thanks for your quick response:)

Thank you for the patch


> On 12/23/2015 08:03 PM, Stefano Stabellini wrote:
> > On Wed, 23 Dec 2015, Cao jin wrote:
> > > Signed-off-by: Cao jin <address@hidden>
> > > ---
> > > 
> > > 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;)

Fine by me


> > Also it might be nice to split the patch in a series.
> > 
> 
> Yup, and the patches should be independent from each other?

That would be best: each patch has to compile independently.


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

I bet you don't have Xen PCI passthrough enabled. Do you have
CONFIG_XEN_PCI_PASSTHROUGH=y in i386-softmmu/config-target.mak?


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

Sound good


> for all the other comments below: will fix them up:)

Thanks!


> > >       }
> > > 
> > >   out:
> > >       close(fd);
> > > -    return rc;
> > >   }
> > > 
> [...]
> > 
> 
> -- 
> Yours Sincerely,
> 
> Cao Jin
> 
>