Re: [Xen-devel] [PATCH v1 02/11] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Nov 03, 2015 at 06:15:59PM +0000, Ross Lagerwall wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

It is very hard for me to review my own code, but mostly
what I see are some quite simple things (really really simple)

> diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
> new file mode 100644
> index 0000000..d984c8a
> --- /dev/null
> +++ b/xen/common/xsplice.c
> +struct payload {
> +    int32_t state;     /* One of XSPLICE_STATE_*. */
> +    int32_t rc;         /* 0 or -EXX. */
> +
> +    struct list_head   list;   /* Linked to 'payload_list'. */
> +
> +    char  id[XEN_XSPLICE_NAME_SIZE + 1];          /* Name of it. */
> +};

There is some space between char and id. Actually all of these look
like they were at the same indentation but are now a bit off.

It would be nice to have them align (the comments) and the
space between 'list_head   list' and 'char  id' shorten.


..snip..
> +int find_payload(xen_xsplice_id_t *id, bool_t need_lock, struct payload **f)
> +{
> +    struct payload *data;
> +    XEN_GUEST_HANDLE_PARAM(char) str;
> +    char name[XEN_XSPLICE_NAME_SIZE + 1] = { 0 }; /* 128 + 1 bytes on stack. 
> Perhaps kzalloc? */

Jan was not too worried about the kzalloc so I think we can
remove the comment.

> +    int rc = -EINVAL;
> +
> +    rc = verify_id(id);
> +    if ( rc )
> +        return rc;
> +
> +    str = guest_handle_cast(id->name, char);
> +    if ( copy_from_guest(name, str, id->size) )
> +        return -EFAULT;
> +
> +    if ( need_lock )
> +        spin_lock(&payload_list_lock);
> +
> +    rc = -ENOENT;
> +    list_for_each_entry ( data, &payload_list, list )
> +    {
> +        if ( !strcmp(data->id, name) )
> +        {
> +            *f = data;
> +            rc = 0;
> +            break;
> +        }
> +    }
> +
> +    if ( need_lock )
> +        spin_unlock(&payload_list_lock);
> +
> +    return rc;
> +}
> +
> +

And we have an extra \n here. 

.. And besides that I realized that there is some code for which
we have CONFIG_ around (lock profile, gcov, kdump?, etc). We
should probably make the xSplice code also be guarded by this
as well.

Ross, I can do these changes easily. Unless you are itching
to do it now :-)

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