[libvirt] [PATCH] Strict check when listing domains

[Matthias Bolte]

2010/3/30 Eduardo Otubo <otubo at linux.vnet.ibm.com>:
>
> I am really sorry. I sent an older version of the patch, here is the correct
> one.
>
>
> Eric Blake wrote:
>>
>> On 03/24/2010 11:19 AM, Eduardo Otubo wrote:
>>
>>>     /* exit early if there are no domains */
>>> -    if (nids == 0)
>>> +    if (nids_numdomains == 0 || nids_listdomains == 0
>>> +        || nids_numdomains != nids_listdomains)
>>>         return 0;
>>
>> Should we be reporting an error here...
>>
>>>  -    if (VIR_ALLOC_N(ids, nids) < 0) {
>>> +    if (VIR_ALLOC_N(ids, nids_listdomains) < 0) {
>>>         virReportOOMError();
>>>         goto err;
>>>     }
>>>  -    if ((nids = phypListDomainsGeneric(conn, ids, nids, 1)) < 0)
>>> -        goto err;
>>> -
>>> -    /* exit early if there are no domains */
>>> -    /* FIXME: phypNumDomainsGeneric() returned > 0 but
>>> phypListDomainsGeneric()
>>> -     *        returned 0. indicates this an error condition?
>>> -     *        an even stricter check would be to treat
>>> -     *
>>> -     *          phypNumDomainsGeneric() != phypListDomainsGeneric()
>>> -     *
>>> -     *        as an error */
>>
>> ...given this comment?  You treated it as an early exit of 0, even if
>> both values were non-zero.
>>
>

Sorry for the late review...

> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index 4f7efdb..eb6be32 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
> @@ -1849,41 +1849,32 @@ phypUUIDTable_Init(virConnectPtr conn)
>  {
>      uuid_tablePtr uuid_table;
>      phyp_driverPtr phyp_driver;
> -    int nids = 0;
> +    int nids_numdomains = 0;
> +    int nids_listdomains = 0;
>      int *ids = NULL;
>      unsigned int i = 0;
>
> -    if ((nids = phypNumDomainsGeneric(conn, 2)) < 0)
> +    if ((nids_numdomains = phypNumDomainsGeneric(conn, 2)) < 0)
> +        goto err;
> +
> +    if ((nids_listdomains =
> +         phypListDomainsGeneric(conn, ids, nids_listdomains, 1)) < 0)

I think you want to call

  phypListDomainsGeneric(conn, ids, nids_numdomains, 1)

instead of

  phypListDomainsGeneric(conn, ids, nids_listdomains, 1)

Also, you have to move the allocation of ids before the call to
phypListDomainsGeneric. Otherwise you're calling
phypListDomainsGeneric with ids = NULL resulting in a segfault.
Because you currently pass nids_listdomains (which is always 0 at this
point) to phypListDomainsGeneric you don't trigger the segfault.

>          goto err;
>
>      /* exit early if there are no domains */
> -    if (nids == 0)
> +    if (nids_numdomains != nids_listdomains)

You should report an error here, otherwise an "unknown error" will be
reported by libvirt.

> +        goto err;
> +    else if (nids_numdomains == 0 && nids_listdomains == 0)
>          return 0;

Matthias