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