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

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

[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>
---
 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)
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH +STABLE-0.14] exit if -drive specified is invalid instead of ignoring the "wrong" -drive

[Markus Armbruster]

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

Re: [Qemu-devel] [PATCH +STABLE-0.14] exit if -drive specified is invalid instead of ignoring the "wrong" -drive

[Michael Tokarev]

17.03.2011 16:51, Markus Armbruster wrote:
> 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.

Yes it doesn't, since to 0.14 the code changed.
What I mean is that the same problem exist in
earlier versions too.  I'll apply a change of
this effect to 0.12.5 as used in Debian now,
something like the one below.

[]
> What about all the other unchecked drive_add() calls in main()?

These are much less worrisome - they fail only of the
internal definitions of options are incorrect, which
should never happen.  For example:

            case QEMU_OPTION_hdd:
                drive_add(IF_DEFAULT, popt->index - QEMU_OPTION_hda, optarg,
                          HD_OPTS);

There, optarg is just a filename, and HD_OPTS is
defined like this:

#define HD_OPTS "media=disk"

So it should not fail when parsing options, only
when trying to interpret and actually open the
filename, which happens much later in the game.

Thanks,

/mjt

For 0.12 and 0.13:

diff --git a/vl.c b/vl.c
index 77677e8..069a1df 100644
--- a/vl.c
+++ b/vl.c
@@ -5025,7 +5025,8 @@ int main(int argc, char **argv, char **envp)
                 drive_add(optarg, HD_ALIAS, popt->index - QEMU_OPTION_hda);
                 break;
             case QEMU_OPTION_drive:
-                drive_add(NULL, "%s", optarg);
+                if (drive_add(NULL, "%s", optarg) == NULL)
+                    exit(1);
                break;
             case QEMU_OPTION_set:
                 if (qemu_set_option(optarg) != 0)

Re: [Qemu-devel] [PATCH +STABLE-0.14] exit if -drive specified is invalid instead of ignoring the "wrong" -drive

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 17.03.2011 16:51, Markus Armbruster wrote:
>> 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.
>
> Yes it doesn't, since to 0.14 the code changed.
> What I mean is that the same problem exist in
> earlier versions too.  I'll apply a change of
> this effect to 0.12.5 as used in Debian now,
> something like the one below.
>
> []
>> What about all the other unchecked drive_add() calls in main()?
>
> These are much less worrisome - they fail only of the
> internal definitions of options are incorrect, which
> should never happen.  For example:
>
>             case QEMU_OPTION_hdd:
>                 drive_add(IF_DEFAULT, popt->index - QEMU_OPTION_hda, optarg,
>                           HD_OPTS);
>
> There, optarg is just a filename, and HD_OPTS is
> defined like this:
>
> #define HD_OPTS "media=disk"
>
> So it should not fail when parsing options, only
> when trying to interpret and actually open the
> filename, which happens much later in the game.

Fair enough.

Re: [Qemu-devel] [PATCH +STABLE-0.14] exit if -drive specified is invalid instead of ignoring the "wrong" -drive

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH +STABLE-0.14] exit if -drive specified is invalid instead of ignoring the "wrong" -drive

[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.

Re: [Qemu-devel] [PATCH +STABLE-0.14] exit if -drive specified is invalid instead of ignoring the "wrong" -drive

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH +STABLE-0.14] exit if -drive specified is invalid instead of ignoring the "wrong" -drive

[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.

Thanks.

/mjt

Re: [Qemu-devel] [PATCH +STABLE-0.14] exit if -drive specified is invalid instead of ignoring the "wrong" -drive

[Kevin Wolf]

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