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

[Eric Blake <eblake@redhat.com>]

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