[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 1/7] libvirt: add new public API virConnectGetCPUModelNames



On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote:
> The new function virConnectGetCPUModelNames allows to retrieve the list
> of CPU models known by the hypervisor for a specific architecture.
> 
> Signed-off-by: Giuseppe Scrivano <gscrivan redhat com>
> ---

>  /**
> + * virConnectGetCPUModelNames:
> + *
> + * @conn: virConnect connection
> + * @arch: Architecture
> + * @models: NULL terminated array of the CPU models supported for the specified
> + * architecture.  Each element and the array itself must be freed by the caller
> + * with free.

Minor tweak: @models: Pointer to a variable to store the NULL-terminated
array...

> + * @flags: extra flags; not used yet, so callers should always pass 0.
> + *
> + * Get the list of supported CPU models for a specific architecture.
> + *
> + * Returns -1 on error, number of elements in @models on success.
> + */
> +int
> +virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char ***models,
> +                           unsigned int flags)
> +{
> +    VIR_DEBUG("conn=%p, arch=%s, flags=%x", conn, arch, flags);
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECT(conn)) {
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +    virCheckNonNullArgReturn(arch, -1);

Can models usefully be NULL, just for the return value without actually
getting the number of names?  I'll decide that based on the patch to the
.x file...
/me goes and looks

Hmm, patch 3/7 blindly stores into *models, meaning you require a
non-NULL models, and are missing a virCheckNonNullArgReturn(models, -1)
here.  On the other hand, it is inconsistent with all our other *ListAll
functions, which allow a NULL pointer for the sake of using just the
return value to grab a count of valid information (and implemented in
the .x file by a side channel need_results in the _args, and ret in the
_ret struct).  I'm leaning towards consistency, so I'll support that
here, too.

Everything else looks good.  ACK with this squashed in:

diff --git i/src/libvirt.c w/src/libvirt.c
index 159d922..b31b561 100644
--- i/src/libvirt.c
+++ w/src/libvirt.c
@@ -18528,9 +18528,10 @@ error:
  *
  * @conn: virConnect connection
  * @arch: Architecture
- * @models: NULL terminated array of the CPU models supported for the
specified
- * architecture.  Each element and the array itself must be freed by
the caller
- * with free.
+ * @models: Pointer to a variable to store the NULL-terminated array of the
+ *          CPU models supported for the specified architecture.  Each
element
+ *          and the array itself must be freed by the caller with free.
 Pass
+ *          NULL if only the list length is needed.
  * @flags: extra flags; not used yet, so callers should always pass 0.
  *
  * Get the list of supported CPU models for a specific architecture.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]