Re: [Xen-devel] [PATCH v9 04/27] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.04.16 at 17:34, <konrad.wilk@xxxxxxxxxx> wrote:
> The implementation does not actually do any patching.
> 
> It just adds the framework for doing the hypercalls,
> keeping track of ELF payloads, and the basic operations:
>  - query which payloads exist,
>  - query for specific payloads,
>  - check*1, apply*1, replace*1, and unload payloads.
> 
> *1: Which of course in this patch are nops.
> 
> The functionality is disabled on ARM until all arch
> components are implemented.
> 
> Also by default it is disabled until the implementation
> is in place.
> 
> We also use recursive spinlocks to so that the find_payload
> function does not need to have a 'lock' and 'non-lock' variant.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Acked-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>

I'm hesitant to say that, but with all of this:

> v9:
>     s/find_name/get_name/, drop locks when allocating data.
>     Drop conditional expression on copyback
>     Move the allocation on upload outside the spinlock.
>     Add (TECH PREVIEW) to the Kconfig help
>     Return -EINVAL if the CHECK or UNLOAD action is to be performed and the 
> payload
>     state is not in expected state.
>     Print 'c' not 'u' when invoking the keyhandler.

... I'm not sure the earlier R-b can still be considered valid. Andrew?

> +static int get_name(const xen_xsplice_name_t *name, char *n)
> +{
> +    if ( !name->size || name->size > XEN_XSPLICE_NAME_SIZE )
> +        return -EINVAL;
> +
> +    if ( name->pad[0] || name->pad[1] || name->pad[2] )
> +        return -EINVAL;
> +
> +    if ( !guest_handle_okay(name->name, name->size) )
> +        return -EINVAL;
> +
> +    if ( __copy_from_guest(n, name->name, name->size) )
> +        return -EFAULT;

Quoting part of my v8.1 reply:
"Is there a particular reason why you open code copy_from_guest() here?"

> +static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
> +{
> +    struct payload *data, *found;
> +    char n[XEN_XSPLICE_NAME_SIZE];
> +    int rc;
> +
> +    rc = verify_payload(upload, n);
> +    if ( rc )
> +        return rc;
> +
> +    data = xzalloc(struct payload);
> +
> +    spin_lock(&payload_lock);
> +
> +    found = find_payload(n);
> +    if ( IS_ERR(found) )
> +    {
> +        rc = PTR_ERR(found);
> +        goto out;
> +    }
> +    else if ( found )
> +    {
> +        rc = -EEXIST;
> +        goto out;
> +    }
> +
> +    if ( !data )
> +    {
> +        rc = -ENOMEM;
> +        goto out;
> +    }
> +
> +    rc = 0;

rc is already zero by the time we get here.

I also wonder whether the code wouldn't be easier to read if you
used just a sequence of if()/else if() here, without any goto-s.

> +static int xsplice_action(xen_sysctl_xsplice_action_t *action)
> +{
> +    struct payload *data;
> +    char n[XEN_XSPLICE_NAME_SIZE];
> +    int rc;
> +
> +    rc = get_name(&action->name, n);
> +    if ( rc )
> +        return rc;
> +
> +    spin_lock(&payload_lock);
> +
> +    data = find_payload(n);
> +    if ( IS_ERR_OR_NULL(data) )
> +    {
> +        spin_unlock(&payload_lock);
> +
> +        if ( !data )
> +            return -ENOENT;
> +
> +        return PTR_ERR(data);
> +    }
> +
> +    switch ( action->cmd )
> +    {
> +    case XSPLICE_ACTION_CHECK:
> +        if ( data->state == XSPLICE_STATE_CHECKED )
> +        {
> +            /* No implementation yet. */
> +            data->state = XSPLICE_STATE_CHECKED;
> +            data->rc = 0;
> +        } else
> +            rc = -EINVAL;
> +        break;

While according to Ross it looks like this is going to go away anyway,
...

> +    case XSPLICE_ACTION_UNLOAD:
> +        if ( data->state == XSPLICE_STATE_CHECKED )
> +        {
> +            free_payload(data);
> +            /* No touching 'data' from here on! */
> +            data = NULL;
> +        } else

... there's a coding style issue here (and also above if that code is
to stay).

> +static const char *state2str(uint32_t state)

Preferably "unsigned int".

> +{
> +#define STATE(x) [XSPLICE_STATE_##x] = #x
> +    static const char *const names[] = {
> +            STATE(CHECKED),
> +            STATE(APPLIED),
> +    };
> +#undef STATE
> +
> +    if (state >= ARRAY_SIZE(names) || !names[state])

Missing blanks.

Jan

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