[libvirt] [libvirt-perl PATCH v2] Virt.xs: fix flag issue on set_scheduler_parameters

From: Alex Jia <Alex Jia ajia@redhat.com> The default flags are inconsistent on both qemuSetSchedulerParameters() and qemuGetSchedulerParameters() in libvirt, the qemuGetSchedulerParameters() always passes 'VIR_DOMAIN_AFFECT_CURRENT' flag to the qemuGetSchedulerParametersFlags(), it should be a expected behavior, but the qemuSetSchedulerParameters() always passes 'VIR_DOMAIN_AFFECT_LIVE' flag to the qemuSetSchedulerParametersFlags(), if users use default flag=0 or explicitly give a 'VIR_DOMAIN_AFFECT_CURRENT' flag to the set_scheduler_parameters() in perl-Sys-Virt, because the flag value is 0, the result is the virDomainSetSchedulerParameters() is called incorrectly. In fact, it's enough to call the qemuSetSchedulerParametersFlags() regardless of what flag is like 'virsh schedinfo' command. How to reproduce? # cat test.pl #!/usr/bin/env perl use warnings; use strict; use Sys::Virt; my $uri = "qemu:///system"; my $domname = "foo"; # change your guest name my $con = Sys::Virt->new(address => $uri, readonly => 0); my $dom = $con->get_domain_by_name($domname); my %sched_param = (Sys::Virt::Domain::SCHEDULER_CPU_SHARES=>1); $dom->set_scheduler_parameters(\%sched_param, Sys::Virt::Domain::AFFECT_CURRENT); # perl test.pl libvirt error code: 55, message: Requested operation is not valid: domain is not running Signed-off-by: Alex Jia <Alex Jia ajia@redhat.com> --- Virt.xs | 10 +++------- 1 files changed, 3 insertions(+), 7 deletions(-) diff --git a/Virt.xs b/Virt.xs index 2b8d74c..fa58cc6 100644 --- a/Virt.xs +++ b/Virt.xs @@ -2833,13 +2833,9 @@ set_scheduler_parameters(dom, newparams, flags=0) } } vir_typed_param_from_hv(newparams, params, nparams); - if (flags) { - if (virDomainSetSchedulerParametersFlags(dom, params, nparams, flags) < 0) - _croak_error(); - } else { - if (virDomainSetSchedulerParameters(dom, params, nparams) < 0) - _croak_error(); - } + + if (virDomainSetSchedulerParametersFlags(dom, params, nparams, flags) < 0) + _croak_error(); Safefree(params); -- 1.7.1

On 08/29/2012 03:03 AM, Alex Jia wrote:
From: Alex Jia <Alex Jia ajia@redhat.com>
The default flags are inconsistent on both qemuSetSchedulerParameters() and qemuGetSchedulerParameters() in libvirt, the qemuGetSchedulerParameters() always passes 'VIR_DOMAIN_AFFECT_CURRENT' flag to the qemuGetSchedulerParametersFlags(), it should be a expected behavior, but the qemuSetSchedulerParameters() always passes 'VIR_DOMAIN_AFFECT_LIVE' flag to the qemuSetSchedulerParametersFlags(), if users use default flag=0 or explicitly give a 'VIR_DOMAIN_AFFECT_CURRENT' flag to the set_scheduler_parameters() in perl-Sys-Virt, because the flag value is 0, the result is the virDomainSetSchedulerParameters() is called incorrectly.
In fact, it's enough to call the qemuSetSchedulerParametersFlags() regardless of what flag is like 'virsh schedinfo' command.
But doing this makes it harder to talk to older libvirtd that lacked the new function. I'm not sure whether we want to blindly force the need for the newer function to exist, or whether we should be reproducing the same logic for deciding when to try the older API based on the flags presented by the user. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Fri, Aug 31, 2012 at 11:49:52AM -0700, Eric Blake wrote:
On 08/29/2012 03:03 AM, Alex Jia wrote:
From: Alex Jia <Alex Jia ajia@redhat.com>
The default flags are inconsistent on both qemuSetSchedulerParameters() and qemuGetSchedulerParameters() in libvirt, the qemuGetSchedulerParameters() always passes 'VIR_DOMAIN_AFFECT_CURRENT' flag to the qemuGetSchedulerParametersFlags(), it should be a expected behavior, but the qemuSetSchedulerParameters() always passes 'VIR_DOMAIN_AFFECT_LIVE' flag to the qemuSetSchedulerParametersFlags(), if users use default flag=0 or explicitly give a 'VIR_DOMAIN_AFFECT_CURRENT' flag to the set_scheduler_parameters() in perl-Sys-Virt, because the flag value is 0, the result is the virDomainSetSchedulerParameters() is called incorrectly.
In fact, it's enough to call the qemuSetSchedulerParametersFlags() regardless of what flag is like 'virsh schedinfo' command.
But doing this makes it harder to talk to older libvirtd that lacked the new function. I'm not sure whether we want to blindly force the need for the newer function to exist, or whether we should be reproducing the same logic for deciding when to try the older API based on the flags presented by the user.
Yeah, the intent of this conditional code in the Perl bindings is to be as conservative as possible. So use the old APIs in all scenarios where they provide the required functionality. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

----- Original Message ----- From: "Daniel P. Berrange" <berrange@redhat.com> To: "Eric Blake" <eblake@redhat.com> Cc: "Alex Jia" <ajia@redhat.com>, libvir-list@redhat.com Sent: Saturday, September 1, 2012 9:14:50 AM Subject: Re: [libvirt] [libvirt-perl PATCH v2] Virt.xs: fix flag issue on set_scheduler_parameters On Fri, Aug 31, 2012 at 11:49:52AM -0700, Eric Blake wrote:
On 08/29/2012 03:03 AM, Alex Jia wrote:
From: Alex Jia <Alex Jia ajia@redhat.com>
The default flags are inconsistent on both qemuSetSchedulerParameters() and qemuGetSchedulerParameters() in libvirt, the qemuGetSchedulerParameters() always passes 'VIR_DOMAIN_AFFECT_CURRENT' flag to the qemuGetSchedulerParametersFlags(), it should be a expected behavior, but the qemuSetSchedulerParameters() always passes 'VIR_DOMAIN_AFFECT_LIVE' flag to the qemuSetSchedulerParametersFlags(), if users use default flag=0 or explicitly give a 'VIR_DOMAIN_AFFECT_CURRENT' flag to the set_scheduler_parameters() in perl-Sys-Virt, because the flag value is 0, the result is the virDomainSetSchedulerParameters() is called incorrectly.
In fact, it's enough to call the qemuSetSchedulerParametersFlags() regardless of what flag is like 'virsh schedinfo' command.
But doing this makes it harder to talk to older libvirtd that lacked the new function. I'm not sure whether we want to blindly force the need for the newer function to exist, or whether we should be reproducing the same logic for deciding when to try the older API based on the flags presented by the user.
Yeah, the intent of this conditional code in the Perl bindings is to be as conservative as possible. So use the old APIs in all scenarios where they provide the required functionality. Yeah, we should consider to talk to older libvirtd, but I'm not sure which solution is better one: the one is to change 'flag' or relevant codes in libvirt-perl; the other is to unify flag value such as also passing flag 'VIR_DOMAIN_AFFECT_CURRENT' to qemuSetSchedulerParametersFlags() in libvirt? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Alex Jia
-
Daniel P. Berrange
-
Eric Blake