This one may be a little contentious... This patch does a couple of things... It won't let you set memory if the value is greater that maxmem. It also will lower memory if max memory is set to less than memory.

I think it's a good argument that this checking should be in the hypervisor control daemon. IMO, it should be both here and in the daemon.

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) ?

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?

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 :-)

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 :-)

Which of these does folks prefer (same would apply to SetMaxMem).

ONE ------ cmdSetmem(vshControl * ctl, vshCmd * cmd) { ... kilobytes = vshCommandOptInt(cmd, "kilobytes", &kilobytes); if (kilobytes <= 0) { virDomainFree(dom); vshError(ctl, FALSE, _("Invalid value of %d for memory size"), kilobytes); return FALSE; } if (virDomainGetInfo(dom, &info) != 0) { virDomainFree(dom); vshError(ctl, FALSE, _("Unable to verify MaxMemorySize")); return FALSE; } if (kilobytes > info.maxMem) { virDomainFree(dom); vshError(ctl, FALSE, _("Invalid value of %d for memory size"), kilobytes); return FALSE; } ... }

On 6/15/07, Daniel P. Berrange <berrange(a)redhat.com&gt; wrote: >On Fri, Jun 15, 2007 at 11:30:02AM -0400, Mark Johnson wrote: >> Which of these does folks prefer (same would apply to SetMaxMem). > >I prefer the first. Full error checking & reporting. GetInfo should >never fail under any reasonable scenario. If it does fail its very >unlikely that a SetMemory would then work, so its pointless continuing > update patch attached.