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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

>         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.

> 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?"

Jan

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