[Libvir] [PATCH] setmem checks
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. I have no idea why the patch diff is so big, Below is the diff after it is applied. A little easier to read. Mark [root@fedora libvirt]# diff src/virsh.orig src/virsh.c 1664a1665
virDomainInfo info;
1673a1675,1677
if (virDomainGetInfo(dom, &info) != 0) { info.maxMem = 0; }
1675c1679 < if (kilobytes <= 0) { ---
if ((kilobytes <= 0) || (kilobytes > info.maxMem)) {
1708a1713
virDomainInfo info;
1723a1729,1737
if (virDomainGetInfo(dom, &info) != 0) { info.memory = 0x7fffffff; }
if (kilobytes < info.memory) { if (virDomainSetMemory(dom, kilobytes) != 0) { return FALSE; } }
[root@fedora libvirt]#
On Thu, Jun 14, 2007 at 04:45:49PM -0400, Mark Johnson wrote:
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.
Yeah, that's a fairly reasonable argument to make, otherwise you're just pushing the burden onto the admin to call virsh dominfo everytime first.
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.
Surprised the Xen Hypervisor doesn't already apply this logic itself actually. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
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) ? Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
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. 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/
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? 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 :-) Mark.
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/
Daniel Veillard <veillard@redhat.com> wrote:
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 :-)
I agree, since libvirt is in developing. Thanks Atsushi SAKAI
On Fri, Jun 15, 2007 at 09:48:42AM -0400, Daniel Veillard wrote:
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 :-)
Yeah, localization is not a problem - there's a huge team of people in Fedora who get us a very quick turn around on translations. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
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; } ... } TWO ----- 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) { if (kilobytes > info.maxMem) { virDomainFree(dom); vshError(ctl, FALSE, _("Invalid value of %d for memory size"), kilobytes); return FALSE; } } ... }
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
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; } ... }
Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On 6/15/07, Daniel P. Berrange <berrange@redhat.com> 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. Mark
On Fri, Jun 15, 2007 at 12:31:41PM -0400, Mark Johnson wrote:
On 6/15/07, Daniel P. Berrange <berrange@redhat.com> 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.
Cool, applied ! thanks a lot ! 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/
Mark Johnson wrote:
On 6/15/07, Daniel P. Berrange <berrange@redhat.com> 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.
+1 Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
participants (5)
-
Atsushi SAKAI -
Daniel P. Berrange -
Daniel Veillard -
Mark Johnson -
Richard W.M. Jones