Re: [Xen-devel] [PATCH v2 1/2] libs, libxc: Interface for grant copy operation

[Paulina Szubarczyk <paulinaszubarczyk@xxxxxxxxx>]

On Fri, 2016-06-17 at 17:43 +0100, Wei Liu wrote:
> On Thu, Jun 16, 2016 at 01:16:54PM +0100, Wei Liu wrote:
> [...]
> [...]
> > > diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
> > > index d286c86..22ad53a 100644
> > > --- a/tools/libs/gnttab/private.h
> > > +++ b/tools/libs/gnttab/private.h
> > > @@ -9,6 +9,20 @@ struct xengntdev_handle {
> > >      int fd;
> > >  };
> > >
> > > +struct xengnttab_copy_grant_segment {
> > > +    union xengnttab_copy_ptr {
> > > +        void *virt;
> > > +        struct {
> > > +            uint32_t ref;
> > > +            uint16_t offset;
> > > +            uint16_t domid;
> > > +        } foreign;
> > > +    } source, dest;
> > > +    uint16_t len;
> > > +    uint16_t flags;
> > > +    int16_t status;
> > > +};
> > >
> >
> > The struct is more or less a direct copy of Linux structure. It is
> > probably fine as-is, but I don't want to risk making this library Linux
> > centric. If you look at other functions, they accept a bunch of discrete
> > arguments then assemble those arguments into OS dependent structure in
> > osdep functions. I know having discrete arguments for the API you want
> > to introduce seems cumbersome, but I want to at least tell you all the
> > background information needed for a thorough discussion. I would be
> > interested in Roger's view on this.
> >
> > I apologise for not having commented on your series earlier.
> >
> 
> After checking various places I'm convinced that this structure is fine
> as-is.
> 
> BSDes have not yet had a user space grant table driver, so I don't
> really worry about that at this point.
> 
> As I have asked you to removed all the stuff in xenctrl_compat.h, you
> will need to move this to libs/gnttab/xengnttab.h.
> 
> Also I have one further comment for code:
> 
> > +int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
> > +                            uint32_t count,
> > +                            xengnttab_grant_copy_segment_t* segs)
> > +{
> > +    int fd = xgt->fd;
> > +    struct ioctl_gntdev_grant_copy copy;
> > +
> > +    copy.segments = (struct ioctl_gntdev_grant_copy_segment*)segs;
> 
> Please create an array of ioctl structure and use explicit field by
> field assignment here.
> 
> Casting like this can easily introduce bug -- just imagine ioctl gets
> changed, or the segment structure gets changed.
> 
> Wei.

Hi Wei, 

Thank you for all the remarks. I am working on correcting the patch.

Paulina



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