Re: [Xen-devel] [PATCH v3 02/23] libxc: Implementation of XEN_XSPLICE_op in libxc (v5).

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

.snip..
> > +/*
> > + * The heart of this function is to get an array of xen_xsplice_status_t.
> > + *
> > + * However it is complex because it has to deal with the hypervisor
> > + * returning -EAGAIN or the data that is being returned becomes stale
> > + * (another hypercall might alter the list).
> > + *
> 
> I don't see EAGAIN handled in the following function. Is that expected?

Wrongly worded. The EAGAIN won't show up - instead it will be the
number of entries potentially less than what is available.
.. snip..
> > +    max_batch_sz = max;
> > +    /* Convience value. */
> > +    sz = sizeof(*name) * XEN_XSPLICE_NAME_SIZE;
> > +    *done = 0;
> > +    *left = 0;
> > +    do {
> > +        /*
> > +         * The first time we go in this loop our 'max' may be bigger
> > +         * than what the hypervisor is comfortable with - hence the first
> > +         * couple of loops may adjust the number of entries we will
> > +         * want filled (tracked by 'nr').
> > +         */
> > +        if ( adjust )
> > +            adjust = 0; /* Used when adjusting the 'max_batch_sz' or 
> > 'retries'. */
> > +
> 
> This is equivalent to always setting adjust to 0.

Correct.
> 
> > +        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 * nr);
> > +        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 + (sz * *done);
> > +        (HYPERCALL_BUFFER(len))->ubuf = len + *done;
> > +        /* Allocate memory. */
> > +        rc = xc_hypercall_bounce_pre(xch, info);
> > +        if ( rc )
> > +            break;
> > +
> > +        rc = xc_hypercall_bounce_pre(xch, name);
> > +        if ( rc )
> > +            break;
> > +
> > +        rc = xc_hypercall_bounce_pre(xch, len);
> > +        if ( rc )
> > +            break;
> > +
> > +        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 )
> > +        {
> > +            /* We could make this configurable as parameter? */
> > +            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. */
> 
> Actually this "adjust" variable looks useless to me because you always
> use "continue" afterwards. It won't ever get used in "while".

We need that for the conditional. Keep in mind that in a do { .. } while
loop the conditional gets checked _after_ the code has run.

Which means (if we did not have adjust) that it would check for:

(*done < max && *left != 0)

And *done = 0, *left = 0 at the start.

Since *left == 0, so we would exit the loop right away.

I've put a comment in the loop about it.
> 
> > +            /* 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! */
> 
> Use EOVERFLOW?

Duh! Yes :-)

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