On Fri, Jun 15, 2007 at 07:52:32AM -0400, Mark Johnson wrote:
> On 6/15/07, Daniel Veillard <veillard@xxxxxxxxxx> 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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/