[Qemu-devel] [PATCH trivial] acpi: actually require either data= or file= for -acpitable

[Qemu-devel] [PATCH trivial] acpi: actually require either data= or file= for -acpitable

[Michael Tokarev]

Initially the code ensured that we have exactly one of
data= or file= option for -acpitable.  But after some
transformations, the condition becomes

  if (has_data == has_file) { error }

to mean, probably, that both should not be set at the same
time.  But this condition does not cover the case when
neither is set, and we generate bogus acpi table in this
case.

Instead, check if sum of the two is exactly 1.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/acpi/core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 42eeace..2815d8c 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -249,7 +249,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
     if (err) {
         goto out;
     }
-    if (hdrs->has_file == hdrs->has_data) {
+    if (!hdrs->has_file + !hdrs->has_data != 1) {
         error_setg(&err, "'-acpitable' requires one of 'data' or 'file'");
         goto out;
     }
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH trivial] acpi: actually require either data= or file= for -acpitable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]

On 06/03/2013 03:20 AM, Michael Tokarev wrote:
> Initially the code ensured that we have exactly one of
> data= or file= option for -acpitable.  But after some
> transformations, the condition becomes
> 
>   if (has_data == has_file) { error }
> 
> to mean, probably, that both should not be set at the same
> time.  But this condition does not cover the case when
> neither is set, and we generate bogus acpi table in this
> case.
> 
> Instead, check if sum of the two is exactly 1.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  hw/acpi/core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 42eeace..2815d8c 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -249,7 +249,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
>      if (err) {
>          goto out;
>      }
> -    if (hdrs->has_file == hdrs->has_data) {

NACK.

hdrs->has_file and hdrs->has_data are both bool.  Pre-patch, the table
of possible combinations is:

false == false => error message, zero given
false == true => okay, exactly one given
true == false => okay, exactly one given
true == true => error message, two given

which is already what you want.

> +    if (!hdrs->has_file + !hdrs->has_data != 1) {

Meanwhile, the post-patch logic is harder to read, in my opinion.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH trivial] acpi: actually require either data= or file= for -acpitable

[Michael Tokarev]

03.06.2013 16:34, Eric Blake wrote:
> On 06/03/2013 03:20 AM, Michael Tokarev wrote:
>> Initially the code ensured that we have exactly one of data= or file= option for -acpitable.  But after some transformations, the condition becomes
>> 
>> if (has_data == has_file) { error }
>> 
>> to mean, probably, that both should not be set at the same time.  But this condition does not cover the case when neither is set, and we generate bogus acpi table in this case.
>> 
>> Instead, check if sum of the two is exactly 1.
>> 
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- hw/acpi/core.c |    2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c index 42eeace..2815d8c 100644 --- a/hw/acpi/core.c +++ b/hw/acpi/core.c @@ -249,7 +249,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp) if (err) { goto out; } -    if (hdrs->has_file == hdrs->has_data) {
> 
> NACK.
> 
> hdrs->has_file and hdrs->has_data are both bool.

Yup.

> Pre-patch, the table of possible combinations is:
> 
> false == false => error message, zero given false == true => okay, exactly one given true == false => okay, exactly one given true == true => error message, two given
> 
> which is already what you want.

Ok, you're right.  Thank you for spotting this!

This function has another error -- if the file specified
(either for data= or file=) can't be read, it happily
continues instead of erroring out.  _That_ is the bug
I tried to hunt but catched something else entirely ;)

Will send a real fix later today... :)

Thanks,

/mjt

Re: [Qemu-devel] [PATCH trivial] acpi: actually require either data= or file= for -acpitable

[Laszlo Ersek]

On 06/03/13 14:42, Michael Tokarev wrote:
> 03.06.2013 16:34, Eric Blake wrote:
>> On 06/03/2013 03:20 AM, Michael Tokarev wrote:
>>> Initially the code ensured that we have exactly one of data= or file= option for -acpitable.  But after some transformations, the condition becomes
>>>
>>> if (has_data == has_file) { error }
>>>
>>> to mean, probably, that both should not be set at the same time.  But this condition does not cover the case when neither is set, and we generate bogus acpi table in this case.
>>>
>>> Instead, check if sum of the two is exactly 1.
>>>
>>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- hw/acpi/core.c |    2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c index 42eeace..2815d8c 100644 --- a/hw/acpi/core.c +++ b/hw/acpi/core.c @@ -249,7 +249,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp) if (err) { goto out; } -    if (hdrs->has_file == hdrs->has_data) {
>>
>> NACK.
>>
>> hdrs->has_file and hdrs->has_data are both bool.
> 
> Yup.
> 
>> Pre-patch, the table of possible combinations is:
>>
>> false == false => error message, zero given false == true => okay, exactly one given true == false => okay, exactly one given true == true => error message, two given
>>
>> which is already what you want.
> 
> Ok, you're right.  Thank you for spotting this!
> 
> This function has another error -- if the file specified
> (either for data= or file=) can't be read, it happily
> continues instead of erroring out.  _That_ is the bug
> I tried to hunt but catched something else entirely ;)
> 
> Will send a real fix later today... :)

Please CC me; IIRC I sought to test this code reasonably deeply when I
wrote / transformed it. I'm curious.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH trivial] acpi: actually require either data= or file= for -acpitable

[Michael Tokarev]

03.06.2013 22:19, Laszlo Ersek wrote:
> On 06/03/13 14:42, Michael Tokarev wrote:
[]
>> This function has another error -- if the file specified
>> (either for data= or file=) can't be read, it happily
>> continues instead of erroring out.  _That_ is the bug
>> I tried to hunt but catched something else entirely ;)
>>
>> Will send a real fix later today... :)
> 
> Please CC me; IIRC I sought to test this code reasonably deeply when I
> wrote / transformed it. I'm curious.

It looks like I used wrong version when encountered this bug.
I can't reproduce it with current source, and by inspecting
the source I don't see any issues with that either.  It made
me to find a non-existing bug earlier, apparently.

Thanks!

/mjt