11:49 a.m.
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
11:49 a.m.
New subject: [libvirt] [PATCH 1/2] virsh: Add check for domain state before attempting resume
---
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
11:49 a.m.
New subject: [libvirt] [PATCH 2/2] virsh: Add check for domain state before attempting suspend
---
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
12:04 p.m.
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 :|
12:23 p.m.
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
12:25 p.m.
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 :|
4240
days inactive
4240
days old
5 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan