Patchwork [+STABLE-0.14] exit if -drive specified is invalid instead of ignoring the "wrong" -drive

login
register
mail settings
Submitter Michael Tokarev
Date March 17, 2011, 7:58 a.m.
Message ID <4D81BF12.4020500@msgid.tls.msk.ru>
Download mbox | patch
Permalink /patch/87351/
State New
Headers show

Comments

Michael Tokarev - March 17, 2011, 7:58 a.m.
Trivial patch.  I've sent it yesterday but somehow it didn't
reach the list.

This fixes the problem when qemu continues even if -drive specification
is somehow invalid, resulting in a mess.  Applicable for both current
master and for stable-0.14 (and 0.13 and 0.12 as well).

The prob can actually be seriuos: when you start guest with two drives
and make an error in the specification of one of them, and the guest
has something like a raid array on the two drives, guest may start failing
that array or kick "missing" drives which may result in a mess - this is
what actually happened to me, I did't want a resync at all, and a resync
resulted in re-writing (and allocating) a 4TB virtual drive I used for
testing, which in turn resulted in my filesystem filling up and whole
thing failing badly.  Yes it was just testing VM, I experimented with
larger raid arrays, but the end result was quite, well, unexpected.

Thanks!

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 vl.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Markus Armbruster - March 17, 2011, 1:51 p.m.
Michael Tokarev <mjt@tls.msk.ru> writes:

> Trivial patch.  I've sent it yesterday but somehow it didn't
> reach the list.
>
> This fixes the problem when qemu continues even if -drive specification
> is somehow invalid, resulting in a mess.  Applicable for both current
> master and for stable-0.14 (and 0.13 and 0.12 as well).

Note patch doesn't apply to 0.12 and 0.13.

> The prob can actually be seriuos: when you start guest with two drives
> and make an error in the specification of one of them, and the guest
> has something like a raid array on the two drives, guest may start failing
> that array or kick "missing" drives which may result in a mess - this is
> what actually happened to me, I did't want a resync at all, and a resync
> resulted in re-writing (and allocating) a 4TB virtual drive I used for
> testing, which in turn resulted in my filesystem filling up and whole
> thing failing badly.  Yes it was just testing VM, I experimented with
> larger raid arrays, but the end result was quite, well, unexpected.
>
> Thanks!
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  vl.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 8bcf2ae..79f996e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2098,7 +2098,8 @@ int main(int argc, char **argv, char **envp)
>                            HD_OPTS);
>                  break;
>              case QEMU_OPTION_drive:
> -                drive_def(optarg);
> +                if (drive_def(optarg) == NULL)
> +                    exit(1);
>  	        break;
>              case QEMU_OPTION_set:
>                  if (qemu_set_option(optarg) != 0)

What about all the other unchecked drive_add() calls in main()?
Kevin Wolf - March 17, 2011, 3:04 p.m.
Am 17.03.2011 08:58, schrieb Michael Tokarev:
> Trivial patch.  I've sent it yesterday but somehow it didn't
> reach the list.
> 
> This fixes the problem when qemu continues even if -drive specification
> is somehow invalid, resulting in a mess.  Applicable for both current
> master and for stable-0.14 (and 0.13 and 0.12 as well).
> 
> The prob can actually be seriuos: when you start guest with two drives
> and make an error in the specification of one of them, and the guest
> has something like a raid array on the two drives, guest may start failing
> that array or kick "missing" drives which may result in a mess - this is
> what actually happened to me, I did't want a resync at all, and a resync
> resulted in re-writing (and allocating) a 4TB virtual drive I used for
> testing, which in turn resulted in my filesystem filling up and whole
> thing failing badly.  Yes it was just testing VM, I experimented with
> larger raid arrays, but the end result was quite, well, unexpected.
> 
> Thanks!
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

Before I got a message like this and the guest started anyway:

    qemu-system-x86_64: -drive asdfj: Invalid parameter 'asdfj'

Now it exits like it should, but I don't get an error message any more.
Exiting silently isn't really nice either.

> ---
>  vl.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 8bcf2ae..79f996e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2098,7 +2098,8 @@ int main(int argc, char **argv, char **envp)
>                            HD_OPTS);
>                  break;
>              case QEMU_OPTION_drive:
> -                drive_def(optarg);
> +                if (drive_def(optarg) == NULL)
> +                    exit(1);

Coding style requires braces here.

Kevin
Markus Armbruster - March 17, 2011, 3:33 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 17.03.2011 08:58, schrieb Michael Tokarev:
>> Trivial patch.  I've sent it yesterday but somehow it didn't
>> reach the list.
>> 
>> This fixes the problem when qemu continues even if -drive specification
>> is somehow invalid, resulting in a mess.  Applicable for both current
>> master and for stable-0.14 (and 0.13 and 0.12 as well).
>> 
>> The prob can actually be seriuos: when you start guest with two drives
>> and make an error in the specification of one of them, and the guest
>> has something like a raid array on the two drives, guest may start failing
>> that array or kick "missing" drives which may result in a mess - this is
>> what actually happened to me, I did't want a resync at all, and a resync
>> resulted in re-writing (and allocating) a 4TB virtual drive I used for
>> testing, which in turn resulted in my filesystem filling up and whole
>> thing failing badly.  Yes it was just testing VM, I experimented with
>> larger raid arrays, but the end result was quite, well, unexpected.
>> 
>> Thanks!
>> 
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>
> Before I got a message like this and the guest started anyway:
>
>     qemu-system-x86_64: -drive asdfj: Invalid parameter 'asdfj'
>
> Now it exits like it should, but I don't get an error message any more.
[...]

Are you sure?  I still get the error message in my testing.
Kevin Wolf - March 17, 2011, 3:41 p.m.
Am 17.03.2011 16:33, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 17.03.2011 08:58, schrieb Michael Tokarev:
>>> Trivial patch.  I've sent it yesterday but somehow it didn't
>>> reach the list.
>>>
>>> This fixes the problem when qemu continues even if -drive specification
>>> is somehow invalid, resulting in a mess.  Applicable for both current
>>> master and for stable-0.14 (and 0.13 and 0.12 as well).
>>>
>>> The prob can actually be seriuos: when you start guest with two drives
>>> and make an error in the specification of one of them, and the guest
>>> has something like a raid array on the two drives, guest may start failing
>>> that array or kick "missing" drives which may result in a mess - this is
>>> what actually happened to me, I did't want a resync at all, and a resync
>>> resulted in re-writing (and allocating) a 4TB virtual drive I used for
>>> testing, which in turn resulted in my filesystem filling up and whole
>>> thing failing badly.  Yes it was just testing VM, I experimented with
>>> larger raid arrays, but the end result was quite, well, unexpected.
>>>
>>> Thanks!
>>>
>>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>>
>> Before I got a message like this and the guest started anyway:
>>
>>     qemu-system-x86_64: -drive asdfj: Invalid parameter 'asdfj'
>>
>> Now it exits like it should, but I don't get an error message any more.
> [...]
> 
> Are you sure?  I still get the error message in my testing.

You're right. Instead of using git am to apply the patch, I just edited
vl.c manually and probably messed up. The patch works.

So it's only the braces.

Kevin
Michael Tokarev - March 17, 2011, 3:49 p.m.
17.03.2011 18:04, Kevin Wolf wrote:
> Am 17.03.2011 08:58, schrieb Michael Tokarev:
[]
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2098,7 +2098,8 @@ int main(int argc, char **argv, char **envp)
>>                            HD_OPTS);
>>                  break;
>>              case QEMU_OPTION_drive:
>> -                drive_def(optarg);
>> +                if (drive_def(optarg) == NULL)
>> +                    exit(1);
> 
> Coding style requires braces here.

I'll just stick it into debian package.  I'm not going
to change all the other braces like this around that place.

Thanks.

/mjt
Kevin Wolf - March 17, 2011, 3:55 p.m.
Am 17.03.2011 16:49, schrieb Michael Tokarev:
> 17.03.2011 18:04, Kevin Wolf wrote:
>> Am 17.03.2011 08:58, schrieb Michael Tokarev:
> []
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -2098,7 +2098,8 @@ int main(int argc, char **argv, char **envp)
>>>                            HD_OPTS);
>>>                  break;
>>>              case QEMU_OPTION_drive:
>>> -                drive_def(optarg);
>>> +                if (drive_def(optarg) == NULL)
>>> +                    exit(1);
>>
>> Coding style requires braces here.
> 
> I'll just stick it into debian package.  I'm not going
> to change all the other braces like this around that place.

I'm only asking to add one pair of braces in the line that you touch,
not to change everything in qemu. Shouldn't be that hard...

Kevin

Patch

diff --git a/vl.c b/vl.c
index 8bcf2ae..79f996e 100644
--- a/vl.c
+++ b/vl.c
@@ -2098,7 +2098,8 @@  int main(int argc, char **argv, char **envp)
                           HD_OPTS);
                 break;
             case QEMU_OPTION_drive:
-                drive_def(optarg);
+                if (drive_def(optarg) == NULL)
+                    exit(1);
 	        break;
             case QEMU_OPTION_set:
                 if (qemu_set_option(optarg) != 0)