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

[Laszlo Ersek <lersek@redhat.com>]

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