[libvirt] [PATCH 09/16] conf: Make virDomainDeviceInfoIterate usable without os type

[Laine Stump]

On 02/20/2013 12:06 PM, Peter Krempa wrote:
> Make the iterator function usable in the next patches. Also refactor
> some parts to avoid strcmp if not necessary.
> ---
>  src/conf/domain_conf.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8024bff..421492f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2311,8 +2311,9 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def,
>              return -1;
>      }
>      for (i = 0; i < def->nconsoles ; i++) {
> -        if ((STREQ(def->os.type, "hvm")) && i == 0 &&
> -            def->consoles[i]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL)
> +        if (i == 0 &&
> +            def->consoles[i]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL &&
> +            STREQ_NULLABLE(def->os.type, "hvm"))
>              continue;
>          device.data.chr = def->consoles[i];
>          if (cb(def, &device, &def->consoles[i]->info, opaque) < 0)

Since this bit of code seems so odd and aberrant, and your change is
going to mask the original commit id that made it (and thus make it more
difficult for someone in the future to learn *why* the code is so
strange), I think you should mention the original commit (and maybe even
a bit of its reasoning) in this patch's commit message.

(The original patch is babe7d, and this was done because (just repeating
the info in the commit message) the first console device for hvm domains
is always an alias of a separately defined <serial> device, so this
prevents the callback being called twice on the same device. If that
information is incorrect, then maybe this code is also incorrect.
(Certainly it would be good to eliminate it if we could, although I
can't claim to have a concrete idea how to do that).

(Interestingly, This same thing can also be true for <hostdev> and
<interface>. Every <interface type='hostdev'> has a corresponding entry
in the hostdev array (with the hostdev's virDeviceInfo *pointing* back
at the interface's virDeviceInfo. However, in that case I took the
strategy of checking for this duality during the body of any callback
that cared, rather than polluting a general purpose iterator function
with details of the items being iterated. So maybe that's a start on how
to eliminate it)

At any rate, ACK to the change; the re-ordering won't have any negative
side effects, and using STREQ_NULLABLE seems like a good idea.