[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize()
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize() |
Date: |
Wed, 23 Dec 2015 14:03:46 +0000 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
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
>
>