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

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

On Wed, Apr 27, 2016 at 12:51:34AM -0600, Jan Beulich wrote:
> >>> On 26.04.16 at 19:50, <konrad.wilk@xxxxxxxxxx> wrote:
> > On Tue, Apr 26, 2016 at 04:21:10AM -0600, Jan Beulich wrote:
> >> >>> 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?
> > 
> > I don't know what the criteria is for dropping an Reviewed-by.
> > I am happy to drop it if you would like - but it may be that Andrew
> > is OK with the way he had his review?
> > 
> > Or is this more of your view as maintainer - that is the patch
> > changed considerably (and what is that? percentage of the patch?
> > small amount of the patch? Trivial changes? Dropping code?)?
> 
> Indeed, that's the aspects that matter: _Any_ non-trivial change
> to the area a tag was offered of should lead to the tag getting
> dropped. That is, if you make substantial changes to e.g. non-XSM
> parts but have an XSM ack, that can of course stay.
> 
> Among the above, the obviously (to me) non-trivial changes are
> the ordering adjustment of allocation vs locking.
> 
> >> > +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?"
> > 
> > You mean why I use guest_handle_okay and __copy_from_guest instead of
> > say copy_from_guest?
> > 
> > I think it is an artificat of earlier changes - in which the find_name
> > would only check 'name-size' and then in another function we would
> > just do '__copy_from_guest'. But that is not needed anymore - so let
> > me change it to 'copy_from_guest'
> 
> Right, but that change didn't happen.
> 
> > I thought at some point you asked for that as the check was done for
> > it once and there was no point
> 
> This may well have been in some much earlier version, where the
> two lived in different places. But when they're together, they
> clearly should be folded back.
> 
> >> > +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.
> > 
> > But I do need to free(data) and unlock the spinlock - so having
> > a common code to pass through makes sense.
> > 
> > Unless you mean have an condition on if ( !rc ), and do the normal path?
> > Like so:
> > 
> >     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);
> >     else if ( found )
> >         rc = -EEXIST;
> > 
> >     if ( !rc && !data )
> 
> This can just be "else if ( !data )" afaict.

But then we "lose"
> 
> >         rc = -ENOMEM;
> > 
> >     if ( !rc )
> 
> And this one then just "else".
> 
> >     {
> >         memcpy(data->name, n, strlen(n));
> >         data->state = XSPLICE_STATE_CHECKED;
> >         INIT_LIST_HEAD(&data->list);
> > 
> >         list_add_tail(&data->list, &payload_list);
> >         payload_cnt++;
> >         payload_version++;
> >     }
> > 
> >     spin_unlock(&payload_lock);
> > 
> >     if ( rc )
> >         xfree(data);
> > 
> >     return rc;
> > 
> > 
> > That looks fine here, but in the subsequent patch I have to also
> > check for
> > 
> > if ( __copy_from_guest(raw_data, upload->payload, upload->size) )       
> 
> This could easily be another "else if()" in the chain outlined above.
> 
> > and
> > rc = load_payload_data(data, raw_data, upload->size);
> 
> But I can see that this one would be a little less neat to integrate.

But it is neater than what it has now.
The final product ends up being:

    rc = verify_payload(upload, n);
    if ( rc )
        return rc;

    data = xzalloc(struct payload);
    raw_data = vmalloc(upload->size);

    spin_lock(&payload_lock);

    found = find_payload(n);
    if ( IS_ERR(found) )
        rc = PTR_ERR(found);
    else if ( found )
        rc = -EEXIST;
    else if ( !data || !raw_data )
        rc = -ENOMEM;
    else if ( __copy_from_guest(raw_data, upload->payload, upload->size) )
        rc = -EFAULT;
    else
    {
        memcpy(data->name, n, strlen(n));

        rc = load_payload_data(data, raw_data, upload->size);
        if ( rc )
            goto out;

        data->state = XSPLICE_STATE_CHECKED;
        INIT_LIST_HEAD(&data->list);

        list_add_tail(&data->list, &payload_list);
        payload_cnt++;
        payload_version++;
    }

 out:
    spin_unlock(&payload_lock);

    vfree(raw_data);

    if ( rc )
        xfree(data);

    return rc;

> 
> > and goto statement help a lot there.
> > 
> > I would rather have it the way it is now if you are OK with that?
> 
> As I have tried to express by saying "I also wonder", and as this
> clearly is a matter of taste to some degree, I'm not insisting on
> that alternative code flow. What I'd really like to ask for is
> consistency though: While in the patch here you use
> 
>     if ( ... )
>     {
>         rc = ...;
>         goto ...;
>     }
> 
> patch 11 introduces an instance of the alternative
> 
>     rc = -E...;
>     if ( ... )
>         goto ...;
> 
> Similarly (see above) you should aim at consistency between
> if/else-if chains or chains of just if-s, when each of them ends in an
> unconditional goto (or return, continue, or break, taking a more
> general perspective). Not mixing styles helps avoid (possibly silent)
> questions by readers along the lines of "Is there a reason it's done
> one way here and another way a few lines down?"

Different authors, different matter of taste - as you saw with
the sizeof and this one - Ross and me write code differently.

How do you and Andrew deal with this one?
> 
> Jan

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