Re: [Xen-devel] [PATCH v4] xenbus_client: extend interface to suppurt multi-page ring

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Sun, Nov 10, 2013 at 11:22:53AM +1300, Matthew Daley wrote:
[...]
> > -int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn)
> > +int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
> > +                     unsigned int nr_pages, grant_ref_t *grefs)
> >  {
> > -       int err = gnttab_grant_foreign_access(dev->otherend_id, ring_mfn, 
> > 0);
> > -       if (err < 0)
> > -               xenbus_dev_fatal(dev, err, "granting access to ring page");
> > +       int err;
> > +       int i;
> > +
> > +       for (i = 0; i < nr_pages; i++) {
> > +               unsigned long addr = (unsigned long)vaddr +
> > +                       (PAGE_SIZE * i);
> > +               err = gnttab_grant_foreign_access(dev->otherend_id,
> > +                                                 virt_to_mfn(addr), 0);
> > +               if (err < 0) {
> > +                       xenbus_dev_fatal(dev, err,
> > +                                        "granting access to ring page");
> > +                       goto fail;
> > +               }
> > +               grefs[i] = err;
> > +       }
> > +
> > +       return 0;
> > +
> > +fail:
> > +       for (; i >= 0; i--)
> 
> There's an off-by-one here, causing gnttab_end_foreign_access_ref to
> be called on the first uninitialized (at least in this function) gref.
> 

Good catch.

> > +               gnttab_end_foreign_access_ref(grefs[i], 0);
> >         return err;
[...]
> > +
> > + fail:
> > +       for (i = j = 0; i < nr_grefs; i++) {
> > +               memset(&unmap[i], 0, sizeof(unmap[i]));
> 
> I think you mean unmap[j], not unmap[i] (both times)? If so, you might
> as well only do this in the following if block, not out here
> unconditionally.
> 

Indeed, will move this inside following if block.

Thanks for revewing. :-)

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel