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

2010-10-12 Thread Daniel Veillard 
On Fri, Oct 08, 2010 at 05:45:00PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 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/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

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

2010-10-12 Thread Eric Blake 

On 10/12/2010 08:01 AM, Daniel Veillard wrote:


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;
 }


Or even one step further, and use annotations to mark the function 
arguments as ATTRIBUTE_NONNULL to get compiler checking of your logic.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

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

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

2010-10-12 Thread Daniel P. Berrange 
On Tue, Oct 12, 2010 at 08:16:11AM -0600, Eric Blake wrote:
 On 10/12/2010 08:01 AM, Daniel Veillard wrote:
 
 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;
  }
 
 Or even one step further, and use annotations to mark the function 
 arguments as ATTRIBUTE_NONNULL to get compiler checking of your logic.

That's only true if the client application has the neccessary compile
time warnings enabled during their build. If you annotate a function
with nonnull, the docs say this activates further compiler optimizations.
So I'd be concerned that annotating the public APIs with  nonnull might
let the compiler optimize away that 'params == NULL' check to nothing.
At which point the app using libvirt would be at risk if they had not
enabled compile warnings to activate the annotation

Daniel
-- 
|: Red Hat, Engineering, London-o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org-o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

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

2010-10-08 Thread Nikunj A. Dadhania 
From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com

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.

Signed-off-by: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
---
 src/driver.h   |   14 ++
 src/esx/esx_driver.c   |2 +
 src/libvirt.c  |  104 
 src/libvirt_public.syms|6 +++
 src/lxc/lxc_driver.c   |2 +
 src/openvz/openvz_driver.c |2 +
 src/phyp/phyp_driver.c |2 +
 src/qemu/qemu_driver.c |2 +
 src/remote/remote_driver.c |2 +
 src/test/test_driver.c |2 +
 src/uml/uml_driver.c   |2 +
 src/xen/xen_driver.c   |2 +
 12 files changed, 142 insertions(+), 0 deletions(-)

diff --git a/src/driver.h b/src/driver.h
index e443c1c..32aeb04 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -128,6 +128,18 @@ typedef int
 (*virDrvDomainSetMemory)   (virDomainPtr domain,
  unsigned long memory);
 typedef int
+(*virDrvDomainSetMemoryParameters)
+(virDomainPtr domain,
+ virMemoryParameterPtr params,
+ int nparams,
+ unsigned int flags);
+typedef int
+(*virDrvDomainGetMemoryParameters)
+(virDomainPtr domain,
+ virMemoryParameterPtr params,
+ int *nparams,
+ unsigned int flags);
+typedef int
 (*virDrvDomainGetInfo) (virDomainPtr domain,
  virDomainInfoPtr info);
 typedef int
@@ -575,6 +587,8 @@ struct _virDriver {
 virDrvDomainRevertToSnapshot domainRevertToSnapshot;
 virDrvDomainSnapshotDelete domainSnapshotDelete;
 virDrvQemuDomainMonitorCommand qemuDomainMonitorCommand;
+virDrvDomainSetMemoryParameters domainSetMemoryParameters;
+virDrvDomainGetMemoryParameters domainGetMemoryParameters;
 };
 
 typedef int
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index e382950..e959be2 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -4217,6 +4217,8 @@ static virDriver esxDriver = {
 esxDomainRevertToSnapshot,   /* domainRevertToSnapshot */
 esxDomainSnapshotDelete, /* domainSnapshotDelete */
 NULL,/* qemuDomainMonitorCommand */
+NULL,/* domainSetMemoryParameters */
+NULL,/* domainGetMemoryParameters */
 };
 
 
diff --git a/src/libvirt.c b/src/libvirt.c
index ca383ba..d964a44 100644
--- 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)
+{
+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;
+}
+conn = domain-conn;
+
+if (conn-driver-domainSetMemoryParameters) {
+int ret;
+ret = conn-driver-domainSetMemoryParameters (domain, params, 
nparams, flags);
+if (ret  0)
+goto error;
+return ret;
+}
+
+virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
+
+error:
+virDispatchError(domain-conn);
+return -1;
+}
+
+/**
+ * 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