[Qemu-devel] [PATCH] Add chardev-send-break monitor command

[Qemu-devel] [PATCH] Add chardev-send-break monitor command

[Stefan Fritsch]

Sending a break on a serial console can be useful for debugging the
guest. But not all chardev backends support sending breaks (only telnet
and mux do). The chardev-send-break command allows to send a break even
if using other backends.

Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
---
 chardev/char.c   | 12 ++++++++++++
 hmp-commands.hx  | 16 ++++++++++++++++
 hmp.c            |  8 ++++++++
 hmp.h            |  1 +
 qapi-schema.json | 20 ++++++++++++++++++++
 5 files changed, 57 insertions(+)

diff --git a/chardev/char.c b/chardev/char.c
index 4e24dc39af..fa54f7c915 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1307,6 +1307,18 @@ void qmp_chardev_remove(const char *id, Error **errp)
     object_unparent(OBJECT(chr));
 }
 
+void qmp_chardev_send_break(const char *id, Error **errp)
+{
+    Chardev *chr;
+
+    chr = qemu_chr_find(id);
+    if (chr == NULL) {
+        error_setg(errp, "Chardev '%s' not found", id);
+        return;
+    }
+    qemu_chr_be_event(chr, CHR_EVENT_BREAK);
+}
+
 void qemu_chr_cleanup(void)
 {
     object_unparent(get_chardevs_root());
diff --git a/hmp-commands.hx b/hmp-commands.hx
index e763606fe5..fc8d54b52a 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1745,6 +1745,22 @@ Removes the chardev @var{id}.
 ETEXI
 
     {
+        .name       = "chardev-send-break",
+        .args_type  = "id:s",
+        .params     = "id",
+        .help       = "send break on chardev",
+        .cmd        = hmp_chardev_send_break,
+        .command_completion = chardev_remove_completion,
+    },
+
+STEXI
+@item chardev-send-break id
+@findex chardev-send-break
+Sends break on the chardev @var{id}.
+
+ETEXI
+
+    {
         .name       = "qemu-io",
         .args_type  = "device:B,command:s",
         .params     = "[device] \"[command]\"",
diff --git a/hmp.c b/hmp.c
index ad723903a6..fb2a38b7d6 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2233,6 +2233,14 @@ void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &local_err);
 }
 
+void hmp_chardev_send_break(Monitor *mon, const QDict *qdict)
+{
+    Error *local_err = NULL;
+
+    qmp_chardev_send_break(qdict_get_str(qdict, "id"), &local_err);
+    hmp_handle_error(mon, &local_err);
+}
+
 void hmp_qemu_io(Monitor *mon, const QDict *qdict)
 {
     BlockBackend *blk;
diff --git a/hmp.h b/hmp.h
index d8b94ce9dc..214b2617e7 100644
--- a/hmp.h
+++ b/hmp.h
@@ -103,6 +103,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
 void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
 void hmp_chardev_add(Monitor *mon, const QDict *qdict);
 void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
+void hmp_chardev_send_break(Monitor *mon, const QDict *qdict);
 void hmp_qemu_io(Monitor *mon, const QDict *qdict);
 void hmp_cpu_add(Monitor *mon, const QDict *qdict);
 void hmp_object_add(Monitor *mon, const QDict *qdict);
diff --git a/qapi-schema.json b/qapi-schema.json
index 4b50b652d3..e01dd83dd9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5114,6 +5114,26 @@
 { 'command': 'chardev-remove', 'data': {'id': 'str'} }
 
 ##
+# @chardev-send-break:
+#
+# Send a break to a character device
+#
+# @id: the chardev's ID, must exist
+#
+# Returns: Nothing on success
+#
+# Since: 2.10
+#
+# Example:
+#
+# -> { "execute": "chardev-send-break", "arguments": { "id" : "foo" } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'chardev-send-break', 'data': {'id': 'str'} }
+
+
+##
 # @TpmModel:
 #
 # An enumeration of TPM models
-- 
2.11.0

Re: [Qemu-devel] [PATCH] Add chardev-send-break monitor command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

On 06/05/2017 03:52 AM, Stefan Fritsch wrote:
> Sending a break on a serial console can be useful for debugging the
> guest. But not all chardev backends support sending breaks (only telnet
> and mux do). The chardev-send-break command allows to send a break even
> if using other backends.
> 
> Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
> ---
>  chardev/char.c   | 12 ++++++++++++
>  hmp-commands.hx  | 16 ++++++++++++++++
>  hmp.c            |  8 ++++++++
>  hmp.h            |  1 +
>  qapi-schema.json | 20 ++++++++++++++++++++
>  5 files changed, 57 insertions(+)

Is there an obvious test that we can enhance to add coverage of the new
QMP command?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] Add chardev-send-break monitor command

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]



On 05/06/2017 14:24, Eric Blake wrote:
> On 06/05/2017 03:52 AM, Stefan Fritsch wrote:
>> Sending a break on a serial console can be useful for debugging the
>> guest. But not all chardev backends support sending breaks (only telnet
>> and mux do). The chardev-send-break command allows to send a break even
>> if using other backends.
>>
>> Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
>> ---
>>  chardev/char.c   | 12 ++++++++++++
>>  hmp-commands.hx  | 16 ++++++++++++++++
>>  hmp.c            |  8 ++++++++
>>  hmp.h            |  1 +
>>  qapi-schema.json | 20 ++++++++++++++++++++
>>  5 files changed, 57 insertions(+)
> 
> Is there an obvious test that we can enhance to add coverage of the new
> QMP command?

You could have a new test covering hw/char/serial.c, but I wouldn't let
that hold the patch.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] Add chardev-send-break monitor command

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 05/06/2017 14:24, Eric Blake wrote:
>> On 06/05/2017 03:52 AM, Stefan Fritsch wrote:
>>> Sending a break on a serial console can be useful for debugging the
>>> guest. But not all chardev backends support sending breaks (only telnet
>>> and mux do). The chardev-send-break command allows to send a break even
>>> if using other backends.
>>>
>>> Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
>>> ---
>>>  chardev/char.c   | 12 ++++++++++++
>>>  hmp-commands.hx  | 16 ++++++++++++++++
>>>  hmp.c            |  8 ++++++++
>>>  hmp.h            |  1 +
>>>  qapi-schema.json | 20 ++++++++++++++++++++
>>>  5 files changed, 57 insertions(+)
>> 
>> Is there an obvious test that we can enhance to add coverage of the new
>> QMP command?
>
> You could have a new test covering hw/char/serial.c, but I wouldn't let
> that hold the patch.

Holding patches is pretty much the only leverage I have to get tests for
new stuff :)

Asking for tests that cover all of serial.c wouldn't be fair.  But I am
asking for basic test coverage of new QMP commands.


Message-ID: <871sugkrw5.fsf@dusky.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00296.html

Re: [Qemu-devel] [PATCH] Add chardev-send-break monitor command

[Stefan Fritsch]

On Tue, 6 Jun 2017, Markus Armbruster wrote:
> >> Is there an obvious test that we can enhance to add coverage of the new
> >> QMP command?
> >
> > You could have a new test covering hw/char/serial.c, but I wouldn't let
> > that hold the patch.
> 
> Holding patches is pretty much the only leverage I have to get tests for
> new stuff :)
> 
> Asking for tests that cover all of serial.c wouldn't be fair.  But I am
> asking for basic test coverage of new QMP commands.

One can easily extend tests/test-hmp.c, but AFAICS that only checks that 
qemu does not crash when the command is executed.

But I have managed to extend tests/test-char.c to call 
qmp_chardev_send_break() in the char_file_test() (see below). Is that what 
you had in mind? It seems it is not possible to use 
qtest_qmp_discard_response() from a unit test, so I don't know how to send 
actual json from inside test-char.c

Cheers,
Stefan


diff --git a/tests/test-char.c b/tests/test-char.c
index dfe856cb85..ad20b55f77 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -53,7 +53,9 @@ static void fe_event(void *opaque, int event)
     FeHandler *h = opaque;
 
     h->last_event = event;
-    quit = true;
+    if (event != CHR_EVENT_BREAK) {
+        quit = true;
+    }
 }
 
 #ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
@@ -491,7 +493,7 @@ static void char_file_test(void)
 
         file.in = fifo;
         file.has_in = true;
-        chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
+        chr = qemu_chardev_new("label-file", TYPE_CHARDEV_FILE, &backend,
                                &error_abort);
 
         qemu_chr_fe_init(&be, chr, &error_abort);
@@ -501,6 +503,10 @@ static void char_file_test(void)
                                  fe_event,
                                  &fe, NULL, true);
 
+        g_assert_cmpint(fe.last_event, !=, CHR_EVENT_BREAK);
+        qmp_chardev_send_break("label-file", NULL);
+        g_assert_cmpint(fe.last_event, ==, CHR_EVENT_BREAK);
+
         main_loop();
 
         close(fd);
diff --git a/tests/test-hmp.c b/tests/test-hmp.c
index 99e35ec15a..6dfa0c36e2 100644
--- a/tests/test-hmp.c
+++ b/tests/test-hmp.c
@@ -22,6 +22,7 @@ static int verbose;
 static const char *hmp_cmds[] = {
     "boot_set ndc",
     "chardev-add null,id=testchardev1",
+    "chardev-send-break testchardev1",
     "chardev-remove testchardev1",
     "commit all",
     "cpu-add 1",

Re: [Qemu-devel] [PATCH] Add chardev-send-break monitor command

[Paolo Bonzini]

> >> Is there an obvious test that we can enhance to add coverage of the new
> >> QMP command?
> >
> > You could have a new test covering hw/char/serial.c, but I wouldn't let
> > that hold the patch.
> 
> Holding patches is pretty much the only leverage I have to get tests for
> new stuff :)
> 
> Asking for tests that cover all of serial.c wouldn't be fair.  But I am
> asking for basic test coverage of new QMP commands.

I agree, on the other hand it's not exactly a comparable area.  I am planning
to write a serial qtest for migration as well (which is more complex than this
QMP command) so I might as well write the test for this new QMP command myself
to get my feet wet...

Paolo

Re: [Qemu-devel] [PATCH] Add chardev-send-break monitor command

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

>> >> Is there an obvious test that we can enhance to add coverage of the new
>> >> QMP command?
>> >
>> > You could have a new test covering hw/char/serial.c, but I wouldn't let
>> > that hold the patch.
>> 
>> Holding patches is pretty much the only leverage I have to get tests for
>> new stuff :)
>> 
>> Asking for tests that cover all of serial.c wouldn't be fair.  But I am
>> asking for basic test coverage of new QMP commands.
>
> I agree, on the other hand it's not exactly a comparable area.  I am planning
> to write a serial qtest for migration as well (which is more complex than this
> QMP command) so I might as well write the test for this new QMP command myself
> to get my feet wet...

I'm willing to take a committment from someone I trust in lieu of actual
tests.  Is this one?

Re: [Qemu-devel] [PATCH] Add chardev-send-break monitor command

[Paolo Bonzini]

----- Original Message -----
> From: "Markus Armbruster" <armbru@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Stefan Fritsch" <sf@sfritsch.de>, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" <dgilbert@redhat.com>,
> "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Sent: Wednesday, June 7, 2017 9:06:53 AM
> Subject: Re: [Qemu-devel] [PATCH] Add chardev-send-break monitor command
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> >> >> Is there an obvious test that we can enhance to add coverage of the new
> >> >> QMP command?
> >> >
> >> > You could have a new test covering hw/char/serial.c, but I wouldn't let
> >> > that hold the patch.
> >> 
> >> Holding patches is pretty much the only leverage I have to get tests for
> >> new stuff :)
> >> 
> >> Asking for tests that cover all of serial.c wouldn't be fair.  But I am
> >> asking for basic test coverage of new QMP commands.
> >
> > I agree, on the other hand it's not exactly a comparable area.  I am planning
> > to write a serial qtest for migration as well (which is more complex than this
> > QMP command) so I might as well write the test for this new QMP command
> > myself to get my feet wet...
> 
> I'm willing to take a committment from someone I trust in lieu of actual
> tests.  Is this one?

Sure, though it looks like Stefan also wrote actual tests so we might get
two birds with a stone.

Paolo

Re: [Qemu-devel] [PATCH] Add chardev-send-break monitor command

[Dr. David Alan Gilbert]

* Stefan Fritsch (sf@sfritsch.de) wrote:
> Sending a break on a serial console can be useful for debugging the
> guest. But not all chardev backends support sending breaks (only telnet
> and mux do). The chardev-send-break command allows to send a break even
> if using other backends.
> 
> Signed-off-by: Stefan Fritsch <sf@sfritsch.de>

From the HMP side, it looks OK, you could easily add a line to
tests/test-hmp.c's command list.

But,

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dave

> ---
>  chardev/char.c   | 12 ++++++++++++
>  hmp-commands.hx  | 16 ++++++++++++++++
>  hmp.c            |  8 ++++++++
>  hmp.h            |  1 +
>  qapi-schema.json | 20 ++++++++++++++++++++
>  5 files changed, 57 insertions(+)
> 
> diff --git a/chardev/char.c b/chardev/char.c
> index 4e24dc39af..fa54f7c915 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -1307,6 +1307,18 @@ void qmp_chardev_remove(const char *id, Error **errp)
>      object_unparent(OBJECT(chr));
>  }
>  
> +void qmp_chardev_send_break(const char *id, Error **errp)
> +{
> +    Chardev *chr;
> +
> +    chr = qemu_chr_find(id);
> +    if (chr == NULL) {
> +        error_setg(errp, "Chardev '%s' not found", id);
> +        return;
> +    }
> +    qemu_chr_be_event(chr, CHR_EVENT_BREAK);
> +}
> +
>  void qemu_chr_cleanup(void)
>  {
>      object_unparent(get_chardevs_root());
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e763606fe5..fc8d54b52a 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1745,6 +1745,22 @@ Removes the chardev @var{id}.
>  ETEXI
>  
>      {
> +        .name       = "chardev-send-break",
> +        .args_type  = "id:s",
> +        .params     = "id",
> +        .help       = "send break on chardev",
> +        .cmd        = hmp_chardev_send_break,
> +        .command_completion = chardev_remove_completion,
> +    },
> +
> +STEXI
> +@item chardev-send-break id
> +@findex chardev-send-break
> +Sends break on the chardev @var{id}.
> +
> +ETEXI
> +
> +    {
>          .name       = "qemu-io",
>          .args_type  = "device:B,command:s",
>          .params     = "[device] \"[command]\"",
> diff --git a/hmp.c b/hmp.c
> index ad723903a6..fb2a38b7d6 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2233,6 +2233,14 @@ void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &local_err);
>  }
>  
> +void hmp_chardev_send_break(Monitor *mon, const QDict *qdict)
> +{
> +    Error *local_err = NULL;
> +
> +    qmp_chardev_send_break(qdict_get_str(qdict, "id"), &local_err);
> +    hmp_handle_error(mon, &local_err);
> +}
> +
>  void hmp_qemu_io(Monitor *mon, const QDict *qdict)
>  {
>      BlockBackend *blk;
> diff --git a/hmp.h b/hmp.h
> index d8b94ce9dc..214b2617e7 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -103,6 +103,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
>  void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
>  void hmp_chardev_add(Monitor *mon, const QDict *qdict);
>  void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
> +void hmp_chardev_send_break(Monitor *mon, const QDict *qdict);
>  void hmp_qemu_io(Monitor *mon, const QDict *qdict);
>  void hmp_cpu_add(Monitor *mon, const QDict *qdict);
>  void hmp_object_add(Monitor *mon, const QDict *qdict);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 4b50b652d3..e01dd83dd9 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -5114,6 +5114,26 @@
>  { 'command': 'chardev-remove', 'data': {'id': 'str'} }
>  
>  ##
> +# @chardev-send-break:
> +#
> +# Send a break to a character device
> +#
> +# @id: the chardev's ID, must exist
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 2.10
> +#
> +# Example:
> +#
> +# -> { "execute": "chardev-send-break", "arguments": { "id" : "foo" } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'chardev-send-break', 'data': {'id': 'str'} }
> +
> +
> +##
>  # @TpmModel:
>  #
>  # An enumeration of TPM models
> -- 
> 2.11.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK