Re: [Xen-devel] [PATCH v3 07/23] xsplice: Implement support for applying/reverting/replacing patches. (v5)

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 02/22/2016 03:00 PM, Ross Lagerwall wrote:
> On 02/12/2016 06:05 PM, Konrad Rzeszutek Wilk wrote:
> snip
> > +static void xsplice_do_single(unsigned int total_cpus)
> > +{
> > +    nmi_callback_t saved_nmi_callback;
> > +    struct payload *data, *tmp;
> > +    s_time_t timeout;
> > +    int rc;
> > +
> > +    data = xsplice_work.data;
> > +    timeout = xsplice_work.timeout + NOW();
> > +    if ( xsplice_do_wait(&xsplice_work.semaphore, timeout, total_cpus,
> > +                         "Timed out on CPU semaphore") )
> > +        return;
> > +
> > +    /* "Mask" NMIs. */
> > +    saved_nmi_callback = set_nmi_callback(mask_nmi_callback);
> > +
> > +    /* All CPUs are waiting, now signal to disable IRQs. */
> > +    xsplice_work.ready = 1;
> > +    smp_wmb();
> > +
> > +    atomic_inc(&xsplice_work.irq_semaphore);
> > +    if ( xsplice_do_wait(&xsplice_work.irq_semaphore, timeout,
> > total_cpus,
> > +                         "Timed out on IRQ semaphore.") )
> > +        return;
> > +
> > +    local_irq_disable();
> > +    /* Now this function should be the only one on any stack.
> > +     * No need to lock the payload list or applied list. */
> > +    switch ( xsplice_work.cmd )
> > +    {
> > +    case XSPLICE_ACTION_APPLY:
> > +        rc = apply_payload(data);
> > +        if ( rc == 0 )
> > +            data->state = XSPLICE_STATE_APPLIED;
> > +        break;
> > +    case XSPLICE_ACTION_REVERT:
> > +        rc = revert_payload(data);
> > +        if ( rc == 0 )
> > +            data->state = XSPLICE_STATE_CHECKED;
> > +        break;
> > +    case XSPLICE_ACTION_REPLACE:
> > +        list_for_each_entry_safe_reverse ( data, tmp, &applied_list,
> > list )
> > +        {
> > +            data->rc = revert_payload(data);
> > +            if ( data->rc == 0 )
> > +                data->state = XSPLICE_STATE_CHECKED;
> > +            else
> > +            {
> > +                rc = -EINVAL;
> > +                break;
> > +            }
> > +        }
> You're using data as a loop iterator here but the variable serves
> another purpose outside the loop. That's not gonna end well.
Also above, you've got:
list_for_each_entry_safe_reverse ( data, tmp, &applied_list, list )

but it needs to be:
list_for_each_entry_safe_reverse ( data, tmp, &applied_list, applied_list )

I'm not sure why this was changed from how I had it...

rc is also used uninitialized in the replace path.

--
Ross Lagerwall

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