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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.

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