[PATCH 1/3] kvm tools: add signal placeholder to avoid balloon crash

[PATCH 1/3] kvm tools: add signal placeholder to avoid balloon crash

[Liming Wang]

If "kvm run" without balloon option, use "kvm balloon" may
crash kvm. Add signal placeholder to avoid this.

Signed-off-by: Liming Wang <walimisdev@gmail.com>
---
 tools/kvm/builtin-run.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
index fa5de27..d725834 100644
--- a/tools/kvm/builtin-run.c
+++ b/tools/kvm/builtin-run.c
@@ -280,6 +280,10 @@ static void handle_sigstop(int sig)
 	kvm_cpu__reboot();
 }
 
+static void handle_empty(int sig)
+{
+}
+
 static void *kvm_cpu_thread(void *arg)
 {
 	current_kvm_cpu		= arg;
@@ -469,6 +473,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 	signal(SIGUSR2, handle_sigusr2);
 	signal(SIGKVMSTOP, handle_sigstop);
 	signal(SIGKVMRESUME, handle_sigusr2);
+	/* signal placeholder if not enable balloon */
+	signal(SIGKVMADDMEM, handle_empty);
+	signal(SIGKVMDELMEM, handle_empty);
 
 	nr_online_cpus = sysconf(_SC_NPROCESSORS_ONLN);
 
-- 
1.7.0.4

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

[Liming Wang]

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){
+			bdev.config.num_pages = 0;
+			return;
+		}
+	}
 
 	/* Notify that the configuration space has changed */
 	bdev.isr = VIRTIO_PCI_ISR_CONFIG;
-- 
1.7.0.4

[PATCH 3/3] kvm tools: enable keyboard press repeat for sdl

[Liming Wang]

Set keyboard repeat rate to enable keyboard press repeat.

It means that don't repeat the key value every 50 milliseconds
until 200 milliseconds later when the key is pressed.

Signed-off-by: Liming Wang <walimisdev@gmail.com>
---
 tools/kvm/ui/sdl.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tools/kvm/ui/sdl.c b/tools/kvm/ui/sdl.c
index 088cd29..6320ce7 100644
--- a/tools/kvm/ui/sdl.c
+++ b/tools/kvm/ui/sdl.c
@@ -99,6 +99,8 @@ static void *sdl__thread(void *p)
 	if (!screen)
 		die("Unable to set SDL video mode");
 
+	SDL_EnableKeyRepeat(200, 50);
+
 	for (;;) {
 		SDL_BlitSurface(guest_screen, NULL, screen, NULL);
 		SDL_Flip(screen);
-- 
1.7.0.4

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

[Sasha Levin]

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 also need to wrap the 'if ()' with parenthesis if you add them to
the 'else' case.

> +			bdev.config.num_pages = 0;
> +			return;
> +		}
> +	}
>  
>  	/* Notify that the configuration space has changed */
>  	bdev.isr = VIRTIO_PCI_ISR_CONFIG;

-- 

Sasha.

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

[walimis]

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?

>
>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?

walimis
>
>> +			bdev.config.num_pages = 0;
>> +			return;
>> +		}
>> +	}
>>  
>>  	/* Notify that the configuration space has changed */
>>  	bdev.isr = VIRTIO_PCI_ISR_CONFIG;
>
>-- 
>
>Sasha.
>

Re: [PATCH 1/3] kvm tools: add signal placeholder to avoid balloon crash

[walimis]

On Thu, Aug 11, 2011 at 10:29:05AM +0300, Sasha Levin wrote:
>On Thu, 2011-08-11 at 13:07 +0800, Liming Wang wrote:
>> If "kvm run" without balloon option, use "kvm balloon" may
>> crash kvm. Add signal placeholder to avoid this.
>> 
>> Signed-off-by: Liming Wang <walimisdev@gmail.com>
>> ---
>>  tools/kvm/builtin-run.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>> 
>> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
>> index fa5de27..d725834 100644
>> --- a/tools/kvm/builtin-run.c
>> +++ b/tools/kvm/builtin-run.c
>> @@ -280,6 +280,10 @@ static void handle_sigstop(int sig)
>>  	kvm_cpu__reboot();
>>  }
>>  
>> +static void handle_empty(int sig)
>> +{
>> +}
>> +
>>  static void *kvm_cpu_thread(void *arg)
>>  {
>>  	current_kvm_cpu		= arg;
>> @@ -469,6 +473,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>>  	signal(SIGUSR2, handle_sigusr2);
>>  	signal(SIGKVMSTOP, handle_sigstop);
>>  	signal(SIGKVMRESUME, handle_sigusr2);
>> +	/* signal placeholder if not enable balloon */
>> +	signal(SIGKVMADDMEM, handle_empty);
>
>You can just do this:
>	signal(SIGKVMADDMEM, SIG_IGN);
>
>For both, and avoid the empty handler.
good. New patch will be sent.

walimis
>
>> +	signal(SIGKVMDELMEM, handle_empty);
>>  
>>  	nr_online_cpus = sysconf(_SC_NPROCESSORS_ONLN);
>>  
>
>-- 
>
>Sasha.
>

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

[Liming Wang]

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 |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/kvm/virtio/balloon.c b/tools/kvm/virtio/balloon.c
index 854d04b..941023e 100644
--- a/tools/kvm/virtio/balloon.c
+++ b/tools/kvm/virtio/balloon.c
@@ -220,10 +220,14 @@ static struct ioport_operations virtio_bln_io_ops = {
 
 static void handle_sigmem(int sig)
 {
-	if (sig == SIGKVMADDMEM)
+	if (sig == SIGKVMADDMEM) {
 		bdev.config.num_pages += 256;
-	else
+	} else {
+		if (bdev.config.num_pages < 256){
+			return;
+		}
 		bdev.config.num_pages -= 256;
+	}
 
 	/* Notify that the configuration space has changed */
 	bdev.isr = VIRTIO_PCI_ISR_CONFIG;
-- 
1.7.0.4

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

[Sasha Levin]

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.

[PATCH 1/3] kvm tools: ignore balloon signals by default to avoid balloon crash

[Liming Wang]

If "kvm run" without balloon option, use "kvm balloon" may
crash kvm. So ignore balloon signals by default to avoid this.

Signed-off-by: Liming Wang <walimisdev@gmail.com>
---
 tools/kvm/builtin-run.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
index fa5de27..1ab7122 100644
--- a/tools/kvm/builtin-run.c
+++ b/tools/kvm/builtin-run.c
@@ -469,6 +469,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 	signal(SIGUSR2, handle_sigusr2);
 	signal(SIGKVMSTOP, handle_sigstop);
 	signal(SIGKVMRESUME, handle_sigusr2);
+	/* ignore balloon signal by default if not enable balloon optiion */
+	signal(SIGKVMADDMEM, SIG_IGN);
+	signal(SIGKVMDELMEM, SIG_IGN);
 
 	nr_online_cpus = sysconf(_SC_NPROCESSORS_ONLN);
 
-- 
1.7.0.4

Re: [PATCH 1/3] kvm tools: add signal placeholder to avoid balloon crash

[Sasha Levin]

On Thu, 2011-08-11 at 13:07 +0800, Liming Wang wrote:
> If "kvm run" without balloon option, use "kvm balloon" may
> crash kvm. Add signal placeholder to avoid this.
> 
> Signed-off-by: Liming Wang <walimisdev@gmail.com>
> ---
>  tools/kvm/builtin-run.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
> index fa5de27..d725834 100644
> --- a/tools/kvm/builtin-run.c
> +++ b/tools/kvm/builtin-run.c
> @@ -280,6 +280,10 @@ static void handle_sigstop(int sig)
>  	kvm_cpu__reboot();
>  }
>  
> +static void handle_empty(int sig)
> +{
> +}
> +
>  static void *kvm_cpu_thread(void *arg)
>  {
>  	current_kvm_cpu		= arg;
> @@ -469,6 +473,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>  	signal(SIGUSR2, handle_sigusr2);
>  	signal(SIGKVMSTOP, handle_sigstop);
>  	signal(SIGKVMRESUME, handle_sigusr2);
> +	/* signal placeholder if not enable balloon */
> +	signal(SIGKVMADDMEM, handle_empty);

You can just do this:
	signal(SIGKVMADDMEM, SIG_IGN);

For both, and avoid the empty handler.

> +	signal(SIGKVMDELMEM, handle_empty);
>  
>  	nr_online_cpus = sysconf(_SC_NPROCESSORS_ONLN);
>  

-- 

Sasha.

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

[Pekka Enberg]

On Thu, 11 Aug 2011, 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 |    8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/kvm/virtio/balloon.c b/tools/kvm/virtio/balloon.c
> index 854d04b..941023e 100644
> --- a/tools/kvm/virtio/balloon.c
> +++ b/tools/kvm/virtio/balloon.c
> @@ -220,10 +220,14 @@ static struct ioport_operations virtio_bln_io_ops = {
>
> static void handle_sigmem(int sig)
> {
> -	if (sig == SIGKVMADDMEM)
> +	if (sig == SIGKVMADDMEM) {
> 		bdev.config.num_pages += 256;
> -	else
> +	} else {
> +		if (bdev.config.num_pages < 256){

Space missing before opening brace.

> +			return;
> +		}

Please don't use braces for single line if statements.

I'll fix it up myself. You can use the scripts/checkpatch script to check 
for this kind of trivial coding style issues before sending a patch out.

> 		bdev.config.num_pages -= 256;
> +	}
>
> 	/* Notify that the configuration space has changed */
> 	bdev.isr = VIRTIO_PCI_ISR_CONFIG;
> -- 
> 1.7.0.4
>
>

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

[walimis]

On Thu, Aug 11, 2011 at 04:36:29PM +0300, Pekka Enberg wrote:
>On Thu, 11 Aug 2011, 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 |    8 ++++++--
>>1 files changed, 6 insertions(+), 2 deletions(-)
>>
>>diff --git a/tools/kvm/virtio/balloon.c b/tools/kvm/virtio/balloon.c
>>index 854d04b..941023e 100644
>>--- a/tools/kvm/virtio/balloon.c
>>+++ b/tools/kvm/virtio/balloon.c
>>@@ -220,10 +220,14 @@ static struct ioport_operations virtio_bln_io_ops = {
>>
>>static void handle_sigmem(int sig)
>>{
>>-	if (sig == SIGKVMADDMEM)
>>+	if (sig == SIGKVMADDMEM) {
>>		bdev.config.num_pages += 256;
>>-	else
>>+	} else {
>>+		if (bdev.config.num_pages < 256){
>
>Space missing before opening brace.
>
>>+			return;
>>+		}
>
>Please don't use braces for single line if statements.
>
>I'll fix it up myself. You can use the scripts/checkpatch script to
>check for this kind of trivial coding style issues before sending a
>patch out.
OK, I will take note next time.

walimis
>
>>		bdev.config.num_pages -= 256;
>>+	}
>>
>>	/* Notify that the configuration space has changed */
>>	bdev.isr = VIRTIO_PCI_ISR_CONFIG;
>>-- 
>>1.7.0.4
>>
>>