| Message ID | 1370251243-20352-1-git-send-email-mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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; }
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(-)