From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ross Lagerwall <ross.lagerwall@citrix.com>
Subject: Re: [PATCH v3 07/23] xsplice: Implement support for
 applying/reverting/replacing patches. (v5)
Date: Mon, 22 Feb 2016 17:06:01 +0000
Message-ID: <56CB3FF9.50305@citrix.com>
References: <1455300361-13092-1-git-send-email-konrad.wilk@oracle.com>
	<1455300361-13092-8-git-send-email-konrad.wilk@oracle.com>
	<56CB228F.2090101@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <prvs=853114b97=ross.lagerwall@citrix.com>)
	id 1aXtw5-0003k1-O1
	for xen-devel@lists.xenproject.org; Mon, 22 Feb 2016 17:06:13 +0000
In-Reply-To: <56CB228F.2090101@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: xen-devel@lists.xenproject.org
List-Id: xen-devel@lists.xenproject.org

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