Re: [Libvir] [PATCH] setmem checks

On Fri, Jun 15, 2007 at 07:52:32AM -0400, Mark Johnson wrote: > On 6/15/07, Daniel Veillard <veillard(a)redhat.com&gt; 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 :-)