Re: [Xen-devel] [PATCH v1 07/12] tmem/libxc: Squash XEN_SYSCTL_TMEM_OP_[SET|SAVE]..

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.09.16 at 11:42, <konrad.wilk@xxxxxxxxxx> wrote:
> Hypervisor wise we had to do a bit of casting. The
> 'struct xen_sysctl_tmem_op'->buf variable is a pointer to
> char. Casting that using the guest_handle_cast to a structure
> (struct tmem_client) does not work. Hence we first have to
> cast it a void and then to xen_sysctl_tmem_client_t.
> This could be improved by having an union and in fact the
> patch titled:
> "tmem: Handle 'struct tmem_info' as a seperate field in the"
> does that. It could be squashed in here but that can make
> it harder to review.

Perhaps appropriate parts of that should then be made a prereq
patch for this one?

> --- a/xen/common/tmem_control.c
> +++ b/xen/common/tmem_control.c
> @@ -258,43 +258,58 @@ static int tmemc_list(domid_t cli_id, 
> tmem_cli_va_param_t buf, uint32_t len,
>      return 0;
>  }
>  
> -static int __tmemc_set_var(struct client *client, uint32_t subop, uint32_t 
> arg1)
> +static int __tmemc_set_var(struct client *client, uint32_t subop,
> +                           XEN_GUEST_HANDLE_PARAM(xen_sysctl_tmem_client_t) 
> buf)
>  {
>      domid_t cli_id = client->cli_id;
>      uint32_t old_weight;
> +    xen_sysctl_tmem_client_t info = { };
>  
> -    switch (subop)
> +    ASSERT(client);
> +
> +    if ( subop != XEN_SYSCTL_TMEM_OP_SET_CLIENT_INFO )
> +    {
> +        tmem_client_warn("tmem: unknown subop %d for tmemc_set_var\n", 
> subop);
> +        return -1;
> +    }
> +    if ( copy_from_guest(&info, buf, 1) )
> +        return -EFAULT;
> +
> +    if ( info.weight != client->info.weight )

Ahead of this - no need to check at least the version field (which I
think would have been more obvious if the field got added in this
patch rather than in the earlier one)?

> +    case XEN_SYSCTL_TMEM_OP_GET_CLIENT_INFO:
> +        rc = 0;
> +        info = guest_handle_cast(buf, xen_sysctl_tmem_client_t);
> +        if ( client )
> +        {
> +            if ( copy_to_guest(info, &client->info, 1) )
> +                rc = -EFAULT;
> +        }
> +        else
> +        {
> +            xen_sysctl_tmem_client_t generic = { .version = 
> TMEM_SPEC_VERSION,
> +                                                 .maxpools = 
> MAX_POOLS_PER_DOMAIN };

static const?

> +typedef struct tmem_client xen_sysctl_tmem_client_t;

Please have typedef name and structure tag match (except
obviously for the trailing _t); in particular I'm not sure about
the need for the _sysctl infix.

Jan


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