[libvirt] [PATCH 0/2] Check domain status before virsh suspend or resume
https://bugzilla.redhat.com/show_bug.cgi?id=906644 Added checks to both virsh suspend and virsh resume for the domain to be in a the right state before trying the suspend/resume. Similar checks to examples/domsuspend/suspend.c. Although not stated in case, any thoughts on adding a --force for both? John Ferlan (2): virsh: Add check for domain state before attempting resume virsh: Add check for domain state before attempting suspend tools/virsh-domain.c | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-) -- 1.8.1.4
--- tools/virsh-domain.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4d0cc8f..f41a74b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4530,19 +4530,29 @@ static bool cmdResume(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - bool ret = true; + bool ret = false; const char *name; + int state; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) - return false; + goto cleanup; + if ((state = vshDomainState(ctl, dom, NULL)) < 0) { + vshPrint(ctl, _("Unable to get domain status")); + goto cleanup; + } + if (state != VIR_DOMAIN_PAUSED) { + vshPrint(ctl, _("Domain %s is not paused"), name); + goto cleanup; + } if (virDomainResume(dom) == 0) { vshPrint(ctl, _("Domain %s resumed\n"), name); + ret = true; } else { vshError(ctl, _("Failed to resume domain %s"), name); - ret = false; } +cleanup: virDomainFree(dom); return ret; } -- 1.8.1.4
--- tools/virsh-domain.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f41a74b..a201316 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2627,18 +2627,30 @@ cmdSuspend(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; const char *name; - bool ret = true; + bool ret = false; + int state; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) - return false; + goto cleanup; - if (virDomainSuspend(dom) == 0) { - vshPrint(ctl, _("Domain %s suspended\n"), name); + if ((state = vshDomainState(ctl, dom, NULL)) < 0) { + vshPrint(ctl, _("Unable to get domain status")); + goto cleanup; + } + if (state == VIR_DOMAIN_RUNNING || + state == VIR_DOMAIN_NOSTATE || + state == VIR_DOMAIN_BLOCKED) { + if (virDomainSuspend(dom) == 0) { + vshPrint(ctl, _("Domain %s suspended\n"), name); + ret = true; + } else { + vshError(ctl, _("Failed to suspend domain %s"), name); + } } else { - vshError(ctl, _("Failed to suspend domain %s"), name); - ret = false; + vshPrint(ctl, _("Domain %s is not running"), name); } +cleanup: virDomainFree(dom); return ret; } -- 1.8.1.4
On Mon, Apr 15, 2013 at 12:49:12PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=906644
Added checks to both virsh suspend and virsh resume for the domain to be in a the right state before trying the suspend/resume. Similar checks to examples/domsuspend/suspend.c.
IMHO this is just a pointless bug request. State checks don't belong in virsh for a start, since that makes it inherantly racey. While the drivers do check for whether the domain is running, they explicitly chose not to raise an error if the VM is already paused, when pause is executed & vica-verca 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 :|
On 04/15/2013 11:04 AM, Daniel P. Berrange wrote:
On Mon, Apr 15, 2013 at 12:49:12PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=906644
Added checks to both virsh suspend and virsh resume for the domain to be in a the right state before trying the suspend/resume. Similar checks to examples/domsuspend/suspend.c.
IMHO this is just a pointless bug request. State checks don't belong in virsh for a start, since that makes it inherantly racey. While the drivers do check for whether the domain is running, they explicitly chose not to raise an error if the VM is already paused, when pause is executed & vica-verca
I agree that doing it in virsh is too racy. If anything, we would need to implement new virDomainSuspendFlags() and virDomainResumeFlags() to let the user pass in a flag that controls whether or not they want libvirtd to reject a no-op state change (we can't change the default in libvirtd, for fear of breaking existing clients, but the only way to do a non-racy non-default behavior is to add a flag which requires adding API). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Apr 15, 2013 at 11:23:09AM -0600, Eric Blake wrote:
On 04/15/2013 11:04 AM, Daniel P. Berrange wrote:
On Mon, Apr 15, 2013 at 12:49:12PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=906644
Added checks to both virsh suspend and virsh resume for the domain to be in a the right state before trying the suspend/resume. Similar checks to examples/domsuspend/suspend.c.
IMHO this is just a pointless bug request. State checks don't belong in virsh for a start, since that makes it inherantly racey. While the drivers do check for whether the domain is running, they explicitly chose not to raise an error if the VM is already paused, when pause is executed & vica-verca
I agree that doing it in virsh is too racy. If anything, we would need to implement new virDomainSuspendFlags() and virDomainResumeFlags() to let the user pass in a flag that controls whether or not they want libvirtd to reject a no-op state change (we can't change the default in libvirtd, for fear of breaking existing clients, but the only way to do a non-racy non-default behavior is to add a flag which requires adding API).
I don't think that anyone cares about this in the real world, so IMHO the correct action is to CLOSED -> WONTFIX the bug report. Regards, 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)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan