[Libvir] [PATCH] setmem checks

[Daniel Veillard]

On Fri, Jun 15, 2007 at 07:52:32AM -0400, Mark Johnson wrote:
> On 6/15/07, Daniel Veillard <veillard at redhat.com> wrote:
> >On Fri, Jun 15, 2007 at 08:59:50AM +0100, Richard W.M. Jones wrote:
> >> Mark Johnson wrote:
> >> >1673a1675,1677
> >> >>    if (virDomainGetInfo(dom, &info) != 0) {
> >> >>      info.maxMem = 0;
> >> >>    }
> >> >1675c1679
> >> ><     if (kilobytes <= 0) {
> >> >---
> >> >>    if ((kilobytes <= 0)  || (kilobytes > info.maxMem)) {
> >>
> >> I don't understand this bit.  If virDomainGetInfo fails then it'll
> >> always give an error because kilobytes > info.maxMem (== 0) ?
> >
> >  Agreed, we probably need to better handle the case where virDomainGetInfo
> >fails, there was an actual scenario where we still wanted to try to set the
> >memory anyway, but I can't remember. But the idea of the patch is fine...
> >  I'm not too fond of
> >    info.memory = 0x7fffffff;
> >can we express that value like (((unsigned int) 1 << 31) -1 ) or a standard
> >macro value for MAX_INT ? but that's cosmetic.
> 
> I agree that both of these are very ugly.  For both cases I didn't 
> understand
> how vshCommandOptDomain() would suceed and then virDomainGetInfo()
> would fail.
>
> And if virDomainGetInfo*() would fail, would it be better to not
> continue on, and
> why virDomainSetMemory() would then pass if we did?

  I think it was releated to the case where hypervisor access would not
work (non-root, no proxy), but doing the RPC to xend for setting up the
amount of memory would actually work. It definitely was an edge case.
  
> It would have been better if I returned FALSE instead to forcing a
> failure. I was just
> doing that to get an error message out and not create yet another string 
> which
> would have to be localized. I'm happy to do what folks think is the right
> thing to do here :-)

  Let's not be afraid of having strings to localize :-)

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/