From mboxrd@z Thu Jan  1 00:00:00 1970
From: Sasha Levin <levinsasha928@gmail.com>
Subject: Re: [PATCH 2/3] kvm tools: check negative value of num_pages
Date: Thu, 11 Aug 2011 10:22:50 +0300
Message-ID: <1313047370.3456.3.camel@lappy>
References: <1313039267-25951-1-git-send-email-walimisdev@gmail.com>
	 <1313039267-25951-2-git-send-email-walimisdev@gmail.com>
	 <1313041021.10059.9.camel@lappy>  <20110811054037.GB13190@walimis-desktop>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Cc: Pekka Enberg <penberg@kernel.org>, Ingo Molnar <mingo@elte.hu>,
	Asias He <asias.hejun@gmail.com>, kvm@vger.kernel.org
To: walimis <walimisdev@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from hera.kernel.org ([140.211.167.34]:54229 "EHLO hera.kernel.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750738Ab1HKHYM (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 11 Aug 2011 03:24:12 -0400
Received: from mail-fx0-f46.google.com (mail-fx0-f46.google.com [209.85.161.46])
	by hera.kernel.org (8.14.4/8.14.3) with ESMTP id p7B7O94J012362
	(version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=FAIL)
	for <kvm@vger.kernel.org>; Thu, 11 Aug 2011 07:24:10 GMT
Received: by fxh19 with SMTP id 19so1482672fxh.19
        for <kvm@vger.kernel.org>; Thu, 11 Aug 2011 00:23:50 -0700 (PDT)
In-Reply-To: <20110811054037.GB13190@walimis-desktop>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Thu, 2011-08-11 at 13:40 +0800, walimis wrote:
> On Thu, Aug 11, 2011 at 08:37:01AM +0300, Sasha Levin wrote:
> >On Thu, 2011-08-11 at 13:07 +0800, Liming Wang wrote:
> >> If num_pages is negative, balloon will make kernel crash with
> >> "out of memory". So we check this value to avoid it to be negative.
> >> 
> >> Signed-off-by: Liming Wang <walimisdev@gmail.com>
> >> ---
> >>  tools/kvm/virtio/balloon.c |    7 ++++++-
> >>  1 files changed, 6 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/tools/kvm/virtio/balloon.c b/tools/kvm/virtio/balloon.c
> >> index 854d04b..0223ee4 100644
> >> --- a/tools/kvm/virtio/balloon.c
> >> +++ b/tools/kvm/virtio/balloon.c
> >> @@ -222,8 +222,13 @@ static void handle_sigmem(int sig)
> >>  {
> >>  	if (sig == SIGKVMADDMEM)
> >>  		bdev.config.num_pages += 256;
> >> -	else
> >> +	else {
> >>  		bdev.config.num_pages -= 256;
> >> +		if ((s32)bdev.config.num_pages < 0){
> >
> >imo it's worth doing this check before the decrement instead of casting
> >to signed here.
> You mean that check whether num_pages less than 256 or equal to 0?
> 

If it's less than 256.

> >
> >you also need to wrap the 'if ()' with parenthesis if you add them to
> >the 'else' case.
> Sorry, I'm not clear what that does mean?
> 

It's a coding style thing. If you have braces in one branch you should
have them in all branches.

For example:

if (something)
	do_this();
else {
	do_this();
	do_that();
}

should be:

if (something) {
	do_this();
} else {
	do_this();
	do_that();
}

We follow the kernel coding style, you can find the full documentation
under the kernel tree in Documentation/CodingStyle .
> walimis
> >
> >> +			bdev.config.num_pages = 0;
> >> +			return;
> >> +		}
> >> +	}
> >>  
> >>  	/* Notify that the configuration space has changed */
> >>  	bdev.isr = VIRTIO_PCI_ISR_CONFIG;
> >
> >-- 
> >
> >Sasha.
> >

-- 

Sasha.