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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.11.16 at 15:00, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 25 November 2016 13:51
>> >>> On 18.11.16 at 18:14, <paul.durrant@xxxxxxxxxx> 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.

Jan


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