Re: [PATCH v2 04/13] libxc: Implementation of XEN_XSPLICE_op in libxc (v4).

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Jan 14, 2016 at 04:47:02PM -0500, Konrad Rzeszutek Wilk wrote:
[...]
> +int xc_xsplice_upload(xc_interface *xch,
> +                      char *name,
> +                      char *payload,
> +                      uint32_t size)
> +{
> +    int rc;
> +    DECLARE_SYSCTL;
> +    DECLARE_HYPERCALL_BOUNCE(payload, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> +    DECLARE_HYPERCALL_BOUNCE(name, 0 /* adjust later */, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> +    xen_xsplice_name_t def_name = { .pad = { 0, 0, 0 } };
> +
> +    if ( !name || !payload )
> +        return -1;
> +
> +    def_name.size = strlen(name);
> +    if ( def_name.size > XEN_XSPLICE_NAME_SIZE )
> +        return -1;
> +
> +    HYPERCALL_BOUNCE_SET_SIZE(name, def_name.size );
> +
> +    if ( xc_hypercall_bounce_pre(xch, name) )
> +        return -1;
> +
> +    if ( xc_hypercall_bounce_pre(xch, payload) )
> +        return -1;
> +

xc_hypercall_bounce_pre can allocate memory so please clean up after
failure instead of returning -1 directly.

> +    sysctl.cmd = XEN_SYSCTL_xsplice_op;
> +    sysctl.u.xsplice.cmd = XEN_SYSCTL_XSPLICE_UPLOAD;
> +    sysctl.u.xsplice.pad = 0;
> +    sysctl.u.xsplice.u.upload.size = size;
> +    set_xen_guest_handle(sysctl.u.xsplice.u.upload.payload, payload);
> +
> +    sysctl.u.xsplice.u.upload.name = def_name;
> +    set_xen_guest_handle(sysctl.u.xsplice.u.upload.name.name, name);
> +
> +    rc = do_sysctl(xch, &sysctl);
> +
> +    xc_hypercall_bounce_post(xch, payload);
> +    xc_hypercall_bounce_post(xch, name);
> +
> +    return rc;
> +}
> +
> +int xc_xsplice_get(xc_interface *xch,
> +                   char *name,
> +                   xen_xsplice_status_t *status)
> +{
> +    int rc;
> +    DECLARE_SYSCTL;
> +    DECLARE_HYPERCALL_BOUNCE(name, 0 /*adjust later */, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> +    xen_xsplice_name_t def_name = { .pad = { 0, 0, 0 } };
> +
> +    if ( !name )
> +        return -1;
> +
> +    def_name.size = strlen(name);
> +    if ( def_name.size > XEN_XSPLICE_NAME_SIZE )
> +        return -1;
> +
> +    HYPERCALL_BOUNCE_SET_SIZE(name, def_name.size );
> +
> +    if ( xc_hypercall_bounce_pre(xch, name) )
> +        return -1;
> +
> +    sysctl.cmd = XEN_SYSCTL_xsplice_op;
> +    sysctl.u.xsplice.cmd = XEN_SYSCTL_XSPLICE_GET;
> +    sysctl.u.xsplice.pad = 0;
> +
> +    sysctl.u.xsplice.u.get.status.state = 0;
> +    sysctl.u.xsplice.u.get.status.rc = 0;
> +
> +    sysctl.u.xsplice.u.get.name = def_name;
> +    set_xen_guest_handle(sysctl.u.xsplice.u.get.name.name, name);
> +
> +    rc = do_sysctl(xch, &sysctl);
> +
> +    xc_hypercall_bounce_post(xch, name);
> +
> +    memcpy(status, &sysctl.u.xsplice.u.get.status, sizeof(*status));
> +
> +    return rc;
> +}
> +
> +int xc_xsplice_list(xc_interface *xch, unsigned int max, unsigned int start,
> +                    xen_xsplice_status_t *info,
> +                    char *name, uint32_t *len,
> +                    unsigned int *done,
> +                    unsigned int *left)


Can you please add some comment before this function to document what
each of the parameters means? I have to admit I fail to grok the
algorithm of this function.

> +{
> +    int rc;
> +    DECLARE_SYSCTL;
> +    DECLARE_HYPERCALL_BOUNCE(info, 0 /* adjust later. */, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> +    DECLARE_HYPERCALL_BOUNCE(name, 0 /* adjust later. */, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> +    DECLARE_HYPERCALL_BOUNCE(len, 0 /* adjust later. */, XC_HYPERCALL_BUFFER_BOUNCE_OUT);

Lines too long.

> +    uint32_t max_batch_sz, nr;
> +    uint32_t version = 0, retries = 0;
> +    uint32_t adjust = 0;
> +
> +    if ( !max || !info || !name || !len )
> +        return -1;
> +
> +    sysctl.cmd = XEN_SYSCTL_xsplice_op;
> +    sysctl.u.xsplice.cmd = XEN_SYSCTL_XSPLICE_LIST;
> +    sysctl.u.xsplice.pad = 0;
> +    sysctl.u.xsplice.u.list.version = 0;
> +    sysctl.u.xsplice.u.list.idx = start;
> +    sysctl.u.xsplice.u.list.pad = 0;
> +
> +    max_batch_sz = max;
> +
> +    *done = 0;
> +    *left = 0;
> +    do {
> +        if ( adjust )
> +            adjust = 0; /* Used when adjusting the 'max_batch_sz' or 'retries'. */
> +
> +        nr = min(max - *done, max_batch_sz);
> +
> +        sysctl.u.xsplice.u.list.nr = nr;
> +        /* Fix the size (may vary between hypercalls). */
> +        HYPERCALL_BOUNCE_SET_SIZE(info, nr * sizeof(*info));
> +        HYPERCALL_BOUNCE_SET_SIZE(name, nr * sizeof(*name) * XEN_XSPLICE_NAME_SIZE);

Line too long.

> +        HYPERCALL_BOUNCE_SET_SIZE(len, nr * sizeof(*len));
> +        /* Move the pointer to proper offset into 'info'. */
> +        (HYPERCALL_BUFFER(info))->ubuf = info + *done;
> +        (HYPERCALL_BUFFER(name))->ubuf = name + (sizeof(*name) * XEN_XSPLICE_NAME_SIZE * *done);
> +        (HYPERCALL_BUFFER(len))->ubuf = len + *done;
> +        /* Allocate memory. */
> +        rc = xc_hypercall_bounce_pre(xch, info);
> +        if ( rc )
> +            return rc;
> +
> +        rc = xc_hypercall_bounce_pre(xch, name);
> +        if ( rc )
> +        {
> +            xc_hypercall_bounce_post(xch, info);
> +            return rc;
> +        }
> +        rc = xc_hypercall_bounce_pre(xch, len);
> +        if ( rc )
> +        {
> +            xc_hypercall_bounce_post(xch, info);
> +            xc_hypercall_bounce_post(xch, name);
> +            return rc;
> +        }

Can you just use "break" instead of three "return"s? That should
simplify code a lot.

> +        set_xen_guest_handle(sysctl.u.xsplice.u.list.status, info);
> +        set_xen_guest_handle(sysctl.u.xsplice.u.list.name, name);
> +        set_xen_guest_handle(sysctl.u.xsplice.u.list.len, len);
> +
> +        rc = do_sysctl(xch, &sysctl);
> +        /*
> +         * From here on we MUST call xc_hypercall_bounce. If rc < 0 we
> +         * end up doing it (outside the loop), so using a break is OK.
> +         */
> +        if ( rc < 0 && errno == E2BIG )
> +        {
> +            if ( max_batch_sz <= 1 )
> +                break;
> +            max_batch_sz >>= 1;
> +            adjust = 1; /* For the loop conditional to let us loop again. */
> +            /* No memory leaks! */
> +            xc_hypercall_bounce_post(xch, info);
> +            xc_hypercall_bounce_post(xch, name);
> +            xc_hypercall_bounce_post(xch, len);
> +            continue;
> +        }
> +        else if ( rc < 0 ) /* For all other errors we bail out. */
> +            break;
> +
> +        if ( !version )
> +            version = sysctl.u.xsplice.u.list.version;
> +
> +        if ( sysctl.u.xsplice.u.list.version != version )
> +        {
> +            /* TODO: retries should be configurable? */
> +            if ( retries++ > 3 )
> +            {
> +                rc = -1;
> +                errno = EBUSY;
> +                break;
> +            }
> +            *done = 0; /* Retry from scratch. */
> +            version = sysctl.u.xsplice.u.list.version;
> +            adjust = 1; /* And make sure we continue in the loop. */
> +            /* No memory leaks. */
> +            xc_hypercall_bounce_post(xch, info);
> +            xc_hypercall_bounce_post(xch, name);
> +            xc_hypercall_bounce_post(xch, len);
> +            continue;
> +        }
> +
> +        /* We should never hit this, but just in case. */
> +        if ( rc > nr )
> +        {
> +            errno = EINVAL; /* Overflow! */
> +            rc = -1;
> +            break;
> +        }
> +        *left = sysctl.u.xsplice.u.list.nr; /* Total remaining count. */
> +        /* Copy only up 'rc' of data' - we could add 'min(rc,nr) if desired. */
> +        HYPERCALL_BOUNCE_SET_SIZE(info, (rc * sizeof(*info)));
> +        HYPERCALL_BOUNCE_SET_SIZE(name, (rc * sizeof(*name) * XEN_XSPLICE_NAME_SIZE));

Line too long.

> +        HYPERCALL_BOUNCE_SET_SIZE(len, (rc * sizeof(*len)));
> +        /* Bounce the data and free the bounce buffer. */
> +        xc_hypercall_bounce_post(xch, info);
> +        xc_hypercall_bounce_post(xch, name);
> +        xc_hypercall_bounce_post(xch, len);
> +        /* And update how many elements of info we have copied into. */
> +        *done += rc;
> +        /* Update idx. */
> +        sysctl.u.xsplice.u.list.idx = *done;
> +    } while ( adjust || (*done < max && *left != 0) );
> +
> +    if ( rc < 0 )
> +    {
> +        xc_hypercall_bounce_post(xch, len);
> +        xc_hypercall_bounce_post(xch, name);
> +        xc_hypercall_bounce_post(xch, info);
> +    }
> +
> +    return rc > 0 ? 0 : rc;
> +}
> +
> +static int _xc_xsplice_action(xc_interface *xch,
> +                              char *name,
> +                              unsigned int action,
> +                              uint32_t timeout)
> +{
> +    int rc;
> +    DECLARE_SYSCTL;
> +    DECLARE_HYPERCALL_BOUNCE(name, 0 /* adjust later */, XC_HYPERCALL_BUFFER_BOUNCE_IN);

Line too long.

Wei.