Re: [PATCH v4 02/13] Adding virDomainSetMemoryParameters and virDomainGetMemoryParameters API

[Daniel Veillard <veillard@xxxxxxxxxx>]

On Fri, Oct 08, 2010 at 05:45:00PM +0530, Nikunj A. Dadhania wrote:
> From: Nikunj A. Dadhania <nikunj@xxxxxxxxxxxxxxxxxx>
> 
> Public api to set/get memory tunables supported by the hypervisors.
> RFC: https://www.redhat.com/archives/libvir-list/2010-August/msg00607.html
> 
> v4:
> * Move exporting public API to this patch
> * Add unsigned int flags to the public api for future extensions
> 
> v3:
> * Add domainGetMemoryParamters and NULL in all the driver interface
> 
> v2:
> * Initialize domainSetMemoryParameters to NULL in all the driver interface
>   structure.
> 
[...]
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -3000,6 +3000,110 @@ error:
>  }
>  
>  /**
> + * virDomainSetMemoryParameters:
> + * @domain: pointer to domain object
> + * @params: pointer to memory parameter objects
> + * @nparams: number of memory parameter (this value should be same or
> + *          less than the number of parameters supported)
> + * @flags: currently unused, for future extension
> + *
> + * Change the memory tunables
> + * This function requires privileged access to the hypervisor. 
> + *
> + * Returns -1 in case of error, 0 in case of success.
> + */
> +int 
> +virDomainSetMemoryParameters(virDomainPtr domain,
> +			     virMemoryParameterPtr params,
> +			     int nparams, unsigned int flags)

  I had to remove tabs and trailing spaces at end of lines here.

> +{
> +    virConnectPtr conn;
> +    DEBUG("domain=%p, params=%p, nparams=%d, flags=%u", domain, params, nparams, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +    if (domain->conn->flags & VIR_CONNECT_RO) {
> +        virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        goto error;
> +    }

Seems that params <= 0 or a NULL params are errors in this function
based on the API description, so I prefer to catch those here and added

    if ((nparams <= 0) || (params == NULL)) {
        virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__);
        goto error;
    }

> +    conn = domain->conn;
> +
> +    if (conn->driver->domainSetMemoryParameters) {
[...]
> +/**
> + * virDomainGetMemoryParameters:
> + * @domain: pointer to domain object
> + * @params: pointer to memory parameter object
> + *          (return value, allocated by the caller)
> + * @nparams: pointer to number of memory parameters
> + * @flags: currently unused, for future extension
> + *
> + * Get the memory parameters, the @params array will be filled with the values
> + * equal to the number of parameters suggested by @nparams
> + *
> + * As a special case, if @nparams is zero and @params is NULL, the API will
> + * set the number of parameters supported by the HV in @nparams and return
> + * SUCCESS. 
> + *
> + * This function requires privileged access to the hypervisor. This function
> + * expects the caller to allocate the 

  unterminated comment. I fixed this as
    * expects the caller to allocate the @param

> + * Returns -1 in case of error, 0 in case of success.
> + */
> +int 
> +virDomainGetMemoryParameters(virDomainPtr domain,
> +			     virMemoryParameterPtr params,
> +			     int *nparams, unsigned int flags)
> +{

  same tabs and trailing spaces problems

> +    virConnectPtr conn;
> +    DEBUG("domain=%p, params=%p, nparams=%d, flags=%u", domain, params, (nparams)?*nparams:-1, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +    if (domain->conn->flags & VIR_CONNECT_RO) {
> +        virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        goto error;
> +    }

  in that case params can be NULL, but nparams must not, and we can't
have *nparams < 0 strictly so I'm adding:

    if ((nparams == NULL) || (*nparams < 0)) {
        virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__);
        goto error;
    }

> +    conn = domain->conn;

  That done, patch is fine,

ACK,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel@xxxxxxxxxxxx  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list