Re: [PATCH] Guard a negative value of "virsh setmem" and "virsh setmexmem"

[Masayuki Sunou <fj1826dm@xxxxxxxxxxxxxxxxx>]

Hi Daniel

> This kind of errors must be caught when calling the API, if one casts 
> from a signed to the unsigned long a check must be done at that point
> for negative value by the caller. I'm unsure in what environment you
> got the problem, but that's not the right way to fix it :-)
> 
This patch intends to add a check of the "bytes" value on virsh.

Signed-off-by: Masayuki Sunou <fj1826dm@xxxxxxxxxxxxxxxxx>

Thanks,
Masayuki Sunou.

-------------------------------------------------------------------------------
Index: src/virsh.c
===================================================================
RCS file: /data/cvs/libvirt/src/virsh.c,v
retrieving revision 1.64
diff -u -p -r1.64 virsh.c
--- src/virsh.c	15 Mar 2007 17:24:57 -0000	1.64
+++ src/virsh.c	19 Mar 2007 00:14:12 -0000
@@ -1459,7 +1459,7 @@ cmdSetmem(vshControl * ctl, vshCmd * cmd
         return FALSE;
 
     bytes = vshCommandOptInt(cmd, "bytes", &bytes);
-    if (!bytes) {
+    if (bytes <= 0) {
         virDomainFree(dom);
         return FALSE;
     }
@@ -1502,7 +1502,7 @@ cmdSetmaxmem(vshControl * ctl, vshCmd * 
         return FALSE;
 
     bytes = vshCommandOptInt(cmd, "bytes", &bytes);
-    if (!bytes) {
+    if (bytes <= 0) {
         virDomainFree(dom);
         return FALSE;
     }
-------------------------------------------------------------------------------

In message <20070316103843.GP3127@xxxxxxxxxx>
   "Re:  [PATCH] Guard a negative value of "virsh setmem" and "virsh setmexmem""
   "Daniel Veillard <veillard@xxxxxxxxxx>" wrote:

> On Fri, Mar 16, 2007 at 12:26:18PM +0900, Masayuki Sunou wrote:
> > Hi
> > 
> > "virsh setmem" and "virsh setmexmem" have the problem of setting a very
> > big value in the domain when -1 is set in the domain. 
> > 
> > As a result, I detected the problem of doing abnormal operation of Xen
> > in 64bit machine. (virsh setmem)
> > 
> > Therefore, I contribute the patch that corrects the argument of the
> > function to guard a negative value. 
> [...]
> >  unsigned long		virDomainGetMaxMemory	(virDomainPtr domain);
> >  int			virDomainSetMaxMemory	(virDomainPtr domain,
> > -						 unsigned long memory);
> > +						 int memory);
> >  int			virDomainSetMemory	(virDomainPtr domain,
> > -						 unsigned long memory);
> > +						 int memory);
> >  int			virDomainGetMaxVcpus	(virDomainPtr domain);
> 
>   Sorry you really cannot do that:
>     - it breaks the API (by changing a public interface)
>     - it breaks the ABI (by changing the parameter size on 64bit boxes)
>     - and it makes Set and Get routines inconsistent.
> 
> This kind of errors must be caught when calling the API, if one casts 
> from a signed to the unsigned long a check must be done at that point
> for negative value by the caller. I'm unsure in what environment you
> got the problem, but that's not the right way to fix it :-)
> 
> Daniel
> 
> -- 
> Red Hat Virtualization group http://redhat.com/virtualization/
> Daniel Veillard      | virtualization library  http://libvirt.org/
> veillard@xxxxxxxxxx  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
> http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/
>