Re: [PATCH-for-4.9 v1 6/8] dm_op: convert HVMOP_set_mem_type

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 25 November 2016 14:16
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>; xen-
> devel@lists.xenproject.org; Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Subject: RE: [Xen-devel] [PATCH-for-4.9 v1 6/8] dm_op: convert
> HVMOP_set_mem_type
> 
> >>> On 25.11.16 at 15:00, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 25 November 2016 13:51
> >> >>> On 18.11.16 at 18:14, <paul.durrant@citrix.com> wrote:
> >> > --- a/tools/libxc/xc_misc.c
> >> > +++ b/tools/libxc/xc_misc.c
> >> > @@ -584,28 +584,18 @@ int xc_hvm_modified_memory( int
> >> xc_hvm_set_mem_type(
> >> >      xc_interface *xch, domid_t dom, hvmmem_type_t mem_type,
> uint64_t
> >> first_pfn, uint64_t nr)
> >> >  {
> >> > -    DECLARE_HYPERCALL_BUFFER(struct xen_hvm_set_mem_type,
> arg);
> >> > -    int rc;
> >> > -
> >> > -    arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
> >> > -    if ( arg == NULL )
> >> > -    {
> >> > -        PERROR("Could not allocate memory for xc_hvm_set_mem_type
> >> hypercall");
> >> > -        return -1;
> >> > -    }
> >> > +    struct xen_dm_op op;
> >> > +    struct xen_dm_op_set_mem_type *data;
> >> >
> >> > -    arg->domid        = dom;
> >> > -    arg->hvmmem_type  = mem_type;
> >> > -    arg->first_pfn    = first_pfn;
> >> > -    arg->nr           = nr;
> >> > +    op.op = DMOP_set_mem_type;
> >> > +    data = &op.u.set_mem_type;
> >> >
> >> > -    rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op,
> >> > -                  HVMOP_set_mem_type,
> >> > -                  HYPERCALL_BUFFER_AS_ARG(arg));
> >> > -
> >> > -    xc_hypercall_buffer_free(xch, arg);
> >> > +    data->mem_type = mem_type;
> >> > +    data->first_pfn = first_pfn;
> >> > +    /* NOTE: The following assignment truncates nr to 32-bits */
> >> > +    data->nr = nr;
> >>
> >> What strange a comment. Why don't you - again as done in the
> >> hvmctl series - simply correct the function's parameter type?
> >> (Same for xc_hvm_track_dirty_vram() and
> >> xc_hvm_modified_memory() then.)
> >
> > Because that may cause compiler warnings in clients when they grab the
> new
> > version of the header. I didn't want to have any adverse effect so just
> > commenting that the value was being truncated (as it always has been)
> seemed
> > like the best thing to do.
> 
> Well, maybe the tool stack maintainers think differently now, but
> for those libxc interface changes I had Wei's R-b already back then.
> In any case the present choice of types is plain wrong, and I
> think it's better if consumers of the API get warned about the
> possible truncation by compilers than silently truncating inside the
> library.
> 

Ok, if you already had agreement from a toolstack maintainer then I'll change the header.

  Paul

> Jan


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