Re: [Qemu-devel] [PATCH v3] bugfix: passing reference instead of value

[Michael S. Tsirkin]

On Mon, Jan 04, 2016 at 02:14:48PM +0000, Stefano Stabellini wrote:
> On Sat, 2 Jan 2016, Michael S. Tsirkin wrote:
> > On Sat, Jan 02, 2016 at 04:02:20PM +0800, Cao jin wrote:
> > > Fix the bug introduced by 595a4f07: function host_pci_config_read() 
> > > should be
> > > pass-by-reference, not value.
> > > 
> > > Signed-off-by: Cao jin <address@hidden>
> > > ---
> > > v3 changelog:
> > > 1. Remove cpu_to_le32() since the code only runs on X86.
> > 
> > It really should be le32_to_cpu and a separate patch,
> > but I think it's preferable to have it there
> > since people tend to copy code around.
> > 
> > But in any case, before merging any patches in this function I'd like to
> > hear a response from someone explaining why is this function necessary
> > at all, since it provably never did anything useful.
> 
> If Tiejun's email address bounces, then we are unlikely to get a reply.
> 
> I think that the pass-by-value bug was introduced in one of the
> rebase/resend versions, as the series is very old and originally looked
> very different. I would take the patch as is to fix the obvious bug.

Yes but with this bug in place, we know no one is using this
device. And if no one can be bothered to test it,
maybe we should rip out the code and be done with it.

OTOH Gerd has apparently been looking at making it
work for kvm, maybe this will bring in testers/users.

I'll apply the fix for now.


> This is how the original code looks like:
> 
> http://xenbits.xen.org/gitweb/?p=qemu-xen-traditional.git;a=blob_plain;f=hw/pt-graphics.c;hb=HEAD
> 
> See the function named igd_pci_read.
> 
> 
> 
> 
> 
> > > 
> > >  hw/pci-host/piix.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > index 715208b..924f0fa 100644
> > > --- a/hw/pci-host/piix.c
> > > +++ b/hw/pci-host/piix.c
> > > @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
> > >      {0xa8, 4},  /* SNB: base of GTT stolen memory */
> > >  };
> > >  
> > > -static int host_pci_config_read(int pos, int len, uint32_t val)
> > > +static int host_pci_config_read(int pos, int len, uint32_t *val)
> > >  {
> > >      char path[PATH_MAX];
> > >      int config_fd;
> > > @@ -784,12 +784,14 @@ static int host_pci_config_read(int pos, int len, 
> > > uint32_t val)
> > >          ret = -errno;
> > >          goto out;
> > >      }
> > > +
> > >      do {
> > > -        rc = read(config_fd, (uint8_t *)&val, len);
> > > +        rc = read(config_fd, (uint8_t *)val, len);
> > >      } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> > >      if (rc != len) {
> > >          ret = -errno;
> > >      }
> > > +
> > >  out:
> > >      close(config_fd);
> > >      return ret;
> > > @@ -805,7 +807,7 @@ static int igd_pt_i440fx_initfn(struct PCIDevice 
> > > *pci_dev)
> > >      for (i = 0; i < num; i++) {
> > >          pos = igd_host_bridge_infos[i].offset;
> > >          len = igd_host_bridge_infos[i].len;
> > > -        rc = host_pci_config_read(pos, len, val);
> > > +        rc = host_pci_config_read(pos, len, &val);
> > >          if (rc) {
> > >              return -ENODEV;
> > >          }
> > > -- 
> > > 2.1.0
> > > 
> > > 
> >