| Message ID | 1336143722-15050-4-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 04/05/2012 17:02, Luiz Capitulino ha scritto: > diff --git a/qapi-schema.json b/qapi-schema.json > index 0166ec2..4dbcb26 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -116,12 +116,14 @@ > # > # @shutdown: guest is shut down (and -no-shutdown is in use) > # > +# @suspended: guest is suspended (ACPI S3) > +# > # @watchdog: the watchdog action is configured to pause and has been triggered > ## > { 'enum': 'RunState', > 'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused', > 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm', > - 'running', 'save-vm', 'shutdown', 'watchdog' ] } > + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] } > This breaks QAPI ABI. Not really a breaker for this series, but it shows how we are not yet ready to keep a stable ABI (as opposed to API), and thus any restrictions on adding optional arguments to commands are premature. (And IMO wrong, there are plenty of ways to have versioned symbols in C without breaking the ABI---not talking about ELF symbol versioning). Paolo
On Fri, 04 May 2012 18:39:06 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 04/05/2012 17:02, Luiz Capitulino ha scritto: > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 0166ec2..4dbcb26 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -116,12 +116,14 @@ > > # > > # @shutdown: guest is shut down (and -no-shutdown is in use) > > # > > +# @suspended: guest is suspended (ACPI S3) > > +# > > # @watchdog: the watchdog action is configured to pause and has been triggered > > ## > > { 'enum': 'RunState', > > 'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused', > > 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm', > > - 'running', 'save-vm', 'shutdown', 'watchdog' ] } > > + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] } > > > > This breaks QAPI ABI. > > Not really a breaker for this series, but it shows how we are not yet > ready to keep a stable ABI (as opposed to API), and thus any Having to add a new enum every time a new value is needed is going to be fun.
On 05/04/2012 10:50 AM, Luiz Capitulino wrote: > On Fri, 04 May 2012 18:39:06 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> Il 04/05/2012 17:02, Luiz Capitulino ha scritto: >>> diff --git a/qapi-schema.json b/qapi-schema.json >>> index 0166ec2..4dbcb26 100644 >>> --- a/qapi-schema.json >>> +++ b/qapi-schema.json >>> @@ -116,12 +116,14 @@ >>> # >>> # @shutdown: guest is shut down (and -no-shutdown is in use) >>> # >>> +# @suspended: guest is suspended (ACPI S3) >>> +# >>> # @watchdog: the watchdog action is configured to pause and has been triggered >>> ## >>> { 'enum': 'RunState', >>> 'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused', >>> 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm', >>> - 'running', 'save-vm', 'shutdown', 'watchdog' ] } >>> + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] } >>> >> >> This breaks QAPI ABI. >> >> Not really a breaker for this series, but it shows how we are not yet >> ready to keep a stable ABI (as opposed to API), and thus any > > Having to add a new enum every time a new value is needed is going to be fun. I think Paolo's point was that new values should be added at the end of the list. Your patch, as written, changes 'watchdog' from 13th to 14th; what you should have done is left 'watchdog' at 13th and made 'suspended' be 14th.
On Fri, 04 May 2012 11:07:03 -0600 Eric Blake <eblake@redhat.com> wrote: > On 05/04/2012 10:50 AM, Luiz Capitulino wrote: > > On Fri, 04 May 2012 18:39:06 +0200 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> Il 04/05/2012 17:02, Luiz Capitulino ha scritto: > >>> diff --git a/qapi-schema.json b/qapi-schema.json > >>> index 0166ec2..4dbcb26 100644 > >>> --- a/qapi-schema.json > >>> +++ b/qapi-schema.json > >>> @@ -116,12 +116,14 @@ > >>> # > >>> # @shutdown: guest is shut down (and -no-shutdown is in use) > >>> # > >>> +# @suspended: guest is suspended (ACPI S3) > >>> +# > >>> # @watchdog: the watchdog action is configured to pause and has been triggered > >>> ## > >>> { 'enum': 'RunState', > >>> 'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused', > >>> 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm', > >>> - 'running', 'save-vm', 'shutdown', 'watchdog' ] } > >>> + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] } > >>> > >> > >> This breaks QAPI ABI. > >> > >> Not really a breaker for this series, but it shows how we are not yet > >> ready to keep a stable ABI (as opposed to API), and thus any > > > > Having to add a new enum every time a new value is needed is going to be fun. > > I think Paolo's point was that new values should be added at the end of > the list. Your patch, as written, changes 'watchdog' from 13th to 14th; > what you should have done is left 'watchdog' at 13th and made > 'suspended' be 14th. We don't have a stable QAPI ABI today, and if I'm not missing the point here he's advocating against it. I don't think this series need any changes in that regard.
Il 04/05/2012 19:13, Luiz Capitulino ha scritto: >>>> > >> This breaks QAPI ABI. >>>> > >> >>>> > >> Not really a breaker for this series, but it shows how we are not yet >>>> > >> ready to keep a stable ABI (as opposed to API), and thus any >>> > > >>> > > Having to add a new enum every time a new value is needed is going to be fun. >> > >> > I think Paolo's point was that new values should be added at the end of >> > the list. Your patch, as written, changes 'watchdog' from 13th to 14th; >> > what you should have done is left 'watchdog' at 13th and made >> > 'suspended' be 14th. > > We don't have a stable QAPI ABI today, and if I'm not missing the point > here he's advocating against it. Yes, but Eric's solution would be fine. > I don't think this series need any changes in that regard. I agree. Paolo
On Sat, 05 May 2012 09:55:35 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 04/05/2012 19:13, Luiz Capitulino ha scritto: > >>>> > >> This breaks QAPI ABI. > >>>> > >> > >>>> > >> Not really a breaker for this series, but it shows how we are not yet > >>>> > >> ready to keep a stable ABI (as opposed to API), and thus any > >>> > > > >>> > > Having to add a new enum every time a new value is needed is going to be fun. > >> > > >> > I think Paolo's point was that new values should be added at the end of > >> > the list. Your patch, as written, changes 'watchdog' from 13th to 14th; > >> > what you should have done is left 'watchdog' at 13th and made > >> > 'suspended' be 14th. > > > > We don't have a stable QAPI ABI today, and if I'm not missing the point > > here he's advocating against it. > > Yes, but Eric's solution would be fine. I'm afraid not, we generate a _MAX enum for bound checking. Yet another argument in favor of your first call.
diff --git a/input.c b/input.c index 6b5c2c3..6968b31 100644 --- a/input.c +++ b/input.c @@ -130,7 +130,7 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry) void kbd_put_keycode(int keycode) { - if (!runstate_is_running()) { + if (!runstate_is_running() && !runstate_check(RUN_STATE_SUSPENDED)) { return; } if (qemu_put_kbd_event) { @@ -154,7 +154,7 @@ void kbd_mouse_event(int dx, int dy, int dz, int buttons_state) void *mouse_event_opaque; int width, height; - if (!runstate_is_running()) { + if (!runstate_is_running() && !runstate_check(RUN_STATE_SUSPENDED)) { return; } if (QTAILQ_EMPTY(&mouse_handlers)) { diff --git a/qapi-schema.json b/qapi-schema.json index 0166ec2..4dbcb26 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -116,12 +116,14 @@ # # @shutdown: guest is shut down (and -no-shutdown is in use) # +# @suspended: guest is suspended (ACPI S3) +# # @watchdog: the watchdog action is configured to pause and has been triggered ## { 'enum': 'RunState', 'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused', 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm', - 'running', 'save-vm', 'shutdown', 'watchdog' ] } + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] } ## # @StatusInfo: diff --git a/qmp.c b/qmp.c index a182b51..fee9fb2 100644 --- a/qmp.c +++ b/qmp.c @@ -151,6 +151,8 @@ void qmp_cont(Error **errp) runstate_check(RUN_STATE_SHUTDOWN)) { error_set(errp, QERR_RESET_REQUIRED); return; + } else if (runstate_check(RUN_STATE_SUSPENDED)) { + return; } bdrv_iterate(iostatus_bdrv_it, NULL); diff --git a/vl.c b/vl.c index ae91a8a..a7afc79 100644 --- a/vl.c +++ b/vl.c @@ -366,6 +366,11 @@ static const RunStateTransition runstate_transitions_def[] = { { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED }, { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE }, + { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED }, + { RUN_STATE_RUNNING, RUN_STATE_SUSPENDED }, + { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING }, + { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE }, + { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING }, { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE }, @@ -1420,6 +1425,7 @@ static void qemu_system_suspend(void) { pause_all_vcpus(); notifier_list_notify(&suspend_notifiers, NULL); + runstate_set(RUN_STATE_SUSPENDED); monitor_protocol_event(QEVENT_SUSPEND, NULL); is_suspended = true; } @@ -1447,6 +1453,7 @@ void qemu_system_wakeup_request(WakeupReason reason) if (!(wakeup_reason_mask & (1 << reason))) { return; } + runstate_set(RUN_STATE_RUNNING); monitor_protocol_event(QEVENT_WAKEUP, NULL); notifier_list_notify(&wakeup_notifiers, &reason); reset_requested = 1;
QEMU enters in this state when the guest suspends to ram (S3). This is important so that HMP users and QMP clients can know that the guest is suspended. QMP also has an event for this, but events are not reliable and are limited (ie. a client can connect to QEMU after the event has been emitted). Having a different state for S3 brings a new issue, though. Every device that doesn't run when the VM is stopped but wants to run when the VM is suspended has to check for RUN_STATE_SUSPENDED explicitly. This is the case for the keyboard and mouse devices, for example. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- input.c | 4 ++-- qapi-schema.json | 4 +++- qmp.c | 2 ++ vl.c | 7 +++++++ 4 files changed, 14 insertions(+), 3 deletions(-)