Re: [PATCH 2/3] kvm tools: check negative value of num_pages

[Sasha Levin <levinsasha928@gmail.com>]

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.