[Qemu-devel] [PATCH 0/3]: qmp/hmp: dump-guest-memory fixes

[Qemu-devel] [PATCH 0/3]: qmp/hmp: dump-guest-memory fixes

[Luiz Capitulino]

Please see individual patches for details.

 dump.c           | 13 +++----------
 hmp-commands.hx  |  8 +++-----
 hmp.c            | 11 ++++++++---
 qapi-schema.json | 32 ++++++++++++++++++++------------
 4 files changed, 34 insertions(+), 30 deletions(-)

[Qemu-devel] [PATCH 1/3] qmp: dump-guest-memory: improve schema doc (again)

[Luiz Capitulino]

 o Add a note about memory allocation with paging=true
 o Fix indentation

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qapi-schema.json | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 14e4419..3d93ebe 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1982,26 +1982,33 @@
 # supported on i386 and x86_64.
 #
 # @paging: if true, do paging to get guest's memory mapping. This allows
-# using gdb to process the core file. However, setting @paging to false
-# may be desirable because of two reasons:
+# 	   using gdb to process the core file.
 #
-#   1. The guest may be in a catastrophic state or can have corrupted
-#      memory, which cannot be trusted
-#   2. The guest can be in real-mode even if paging is enabled. For example,
-#      the guest uses ACPI to sleep, and ACPI sleep state goes in real-mode
+# 	   IMPORTANT: this option can make QEMU allocates several gigabytes
+# 	   	      of RAM. This can happen for a large guest, or a
+# 	   	      malicious guest pretending to be large.
+#
+#	   Also, paging=true has the following limitations:
+#
+#          1. The guest may be in a catastrophic state or can have corrupted
+#             memory, which cannot be trusted
+#          2. The guest can be in real-mode even if paging is enabled. For
+#             example, the guest uses ACPI to sleep, and ACPI sleep state
+#             goes in real-mode
 #
 # @protocol: the filename or file descriptor of the vmcore. The supported
-# protocols are:
+# 	     protocols are:
 #
-#   1. file: the protocol starts with "file:", and the following string is
-#      the file's path.
-#   2. fd: the protocol starts with "fd:", and the following string is the
-#      fd's name.
+#            1. file: the protocol starts with "file:", and the following
+#            	string is the file's path.
+#            2. fd: the protocol starts with "fd:", and the following string
+#            	is the fd's name.
 #
 # @begin: #optional if specified, the starting physical address.
 #
 # @length: #optional if specified, the memory size, in bytes. If you don't
-# want to dump all guest's memory, please specify the start @begin and @length
+# 	   want to dump all guest's memory, please specify the start @begin
+# 	   and @length
 #
 # Returns: nothing on success
 #
@@ -2010,6 +2017,7 @@
 { 'command': 'dump-guest-memory',
   'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
             '*length': 'int' } }
+
 ##
 # @netdev_add:
 #
-- 
1.7.12.315.g682ce8b

Re: [Qemu-devel] [PATCH 1/3] qmp: dump-guest-memory: improve schema doc (again)

[Eric Blake]

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

On 09/21/2012 11:07 AM, Luiz Capitulino wrote:
>  o Add a note about memory allocation with paging=true
>  o Fix indentation
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qapi-schema.json | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 14e4419..3d93ebe 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1982,26 +1982,33 @@
>  # supported on i386 and x86_64.
>  #
>  # @paging: if true, do paging to get guest's memory mapping. This allows
> -# using gdb to process the core file. However, setting @paging to false
> -# may be desirable because of two reasons:
> +# 	   using gdb to process the core file.

You've got a TAB in there, that made it hard to see what the intended
indentation really is.  The rest of the file doesn't have TABs, so you
need to respin this.

> +# 	   IMPORTANT: this option can make QEMU allocates several gigabytes

s/allocates/allocate/

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH 2/3] qmp: dump-guest-memory: don't spin if non-blocking fd would block

[Luiz Capitulino]

fd_write_vmcore() will indefinitely spin for a non-blocking
file-descriptor that would block. However, if the fd is non-blocking,
how does it make sense to spin?

Change this behavior to return an error instead.

Note that this can only happen with an fd provided by a management
application. The fd opened internally by dump-guest-memory is blocking.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 dump.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/dump.c b/dump.c
index 2bf8d8d..5eea015 100644
--- a/dump.c
+++ b/dump.c
@@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason)
 static int fd_write_vmcore(void *buf, size_t size, void *opaque)
 {
     DumpState *s = opaque;
-    int fd = s->fd;
     size_t writen_size;
 
-    /* The fd may be passed from user, and it can be non-blocked */
-    while (size) {
-        writen_size = qemu_write_full(fd, buf, size);
-        if (writen_size != size && errno != EAGAIN) {
-            return -1;
-        }
-
-        buf += writen_size;
-        size -= writen_size;
+    writen_size = qemu_write_full(s->fd, buf, size);
+    if (writen_size != size) {
+        return -1;
     }
 
     return 0;
-- 
1.7.12.315.g682ce8b

Re: [Qemu-devel] [PATCH 2/3] qmp: dump-guest-memory: don't spin if non-blocking fd would block

[Eric Blake]

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

On 09/21/2012 11:07 AM, Luiz Capitulino wrote:
> fd_write_vmcore() will indefinitely spin for a non-blocking
> file-descriptor that would block. However, if the fd is non-blocking,
> how does it make sense to spin?
> 
> Change this behavior to return an error instead.
> 
> Note that this can only happen with an fd provided by a management
> application. The fd opened internally by dump-guest-memory is blocking.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  dump.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 2bf8d8d..5eea015 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason)
>  static int fd_write_vmcore(void *buf, size_t size, void *opaque)
>  {
>      DumpState *s = opaque;
> -    int fd = s->fd;
>      size_t writen_size;

While you are here, s/writen/written/ in the local variable.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 2/3] qmp: dump-guest-memory: don't spin if non-blocking fd would block

[Wen Congyang]

At 09/22/2012 01:07 AM, Luiz Capitulino Wrote:
> fd_write_vmcore() will indefinitely spin for a non-blocking
> file-descriptor that would block. However, if the fd is non-blocking,
> how does it make sense to spin?
> 
> Change this behavior to return an error instead.
> 
> Note that this can only happen with an fd provided by a management
> application. The fd opened internally by dump-guest-memory is blocking.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  dump.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 2bf8d8d..5eea015 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason)
>  static int fd_write_vmcore(void *buf, size_t size, void *opaque)
>  {
>      DumpState *s = opaque;
> -    int fd = s->fd;
>      size_t writen_size;
>  
> -    /* The fd may be passed from user, and it can be non-blocked */
> -    while (size) {
> -        writen_size = qemu_write_full(fd, buf, size);
> -        if (writen_size != size && errno != EAGAIN) {

Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the
function doesn't spin. What problems do you meet?

Thanks
Wen Congyang

> -            return -1;
> -        }
> -
> -        buf += writen_size;
> -        size -= writen_size;
> +    writen_size = qemu_write_full(s->fd, buf, size);
> +    if (writen_size != size) {
> +        return -1;
>      }
>  
>      return 0;

Re: [Qemu-devel] [PATCH 2/3] qmp: dump-guest-memory: don't spin if non-blocking fd would block

[Luiz Capitulino]

On Mon, 24 Sep 2012 14:27:17 +0800
Wen Congyang <wency@cn.fujitsu.com> wrote:

> At 09/22/2012 01:07 AM, Luiz Capitulino Wrote:
> > fd_write_vmcore() will indefinitely spin for a non-blocking
> > file-descriptor that would block. However, if the fd is non-blocking,
> > how does it make sense to spin?
> > 
> > Change this behavior to return an error instead.
> > 
> > Note that this can only happen with an fd provided by a management
> > application. The fd opened internally by dump-guest-memory is blocking.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  dump.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/dump.c b/dump.c
> > index 2bf8d8d..5eea015 100644
> > --- a/dump.c
> > +++ b/dump.c
> > @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason)
> >  static int fd_write_vmcore(void *buf, size_t size, void *opaque)
> >  {
> >      DumpState *s = opaque;
> > -    int fd = s->fd;
> >      size_t writen_size;
> >  
> > -    /* The fd may be passed from user, and it can be non-blocked */
> > -    while (size) {
> > -        writen_size = qemu_write_full(fd, buf, size);
> > -        if (writen_size != size && errno != EAGAIN) {
> 
> Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the
> function doesn't spin. What problems do you meet?

The problem is with non-blocking fds, where spinning isn't correct, for
two reasons:

 1. If the fd is non-blocking, that means you don't want to block
    and spinning for a long time will have the same effects

 2. Spinning consumes host resources

Re: [Qemu-devel] [PATCH 2/3] qmp: dump-guest-memory: don't spin if non-blocking fd would block

[Wen Congyang]

At 09/24/2012 09:34 PM, Luiz Capitulino Wrote:
> On Mon, 24 Sep 2012 14:27:17 +0800
> Wen Congyang <wency@cn.fujitsu.com> wrote:
> 
>> At 09/22/2012 01:07 AM, Luiz Capitulino Wrote:
>>> fd_write_vmcore() will indefinitely spin for a non-blocking
>>> file-descriptor that would block. However, if the fd is non-blocking,
>>> how does it make sense to spin?
>>>
>>> Change this behavior to return an error instead.
>>>
>>> Note that this can only happen with an fd provided by a management
>>> application. The fd opened internally by dump-guest-memory is blocking.
>>>
>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> ---
>>>  dump.c | 13 +++----------
>>>  1 file changed, 3 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/dump.c b/dump.c
>>> index 2bf8d8d..5eea015 100644
>>> --- a/dump.c
>>> +++ b/dump.c
>>> @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason)
>>>  static int fd_write_vmcore(void *buf, size_t size, void *opaque)
>>>  {
>>>      DumpState *s = opaque;
>>> -    int fd = s->fd;
>>>      size_t writen_size;
>>>  
>>> -    /* The fd may be passed from user, and it can be non-blocked */
>>> -    while (size) {
>>> -        writen_size = qemu_write_full(fd, buf, size);
>>> -        if (writen_size != size && errno != EAGAIN) {
>>
>> Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the
>> function doesn't spin. What problems do you meet?
> 
> The problem is with non-blocking fds, where spinning isn't correct, for
> two reasons:

But, If the fd is non-blocking, errno can't be EAGAIN. So it doesn't spin.

Thanks
Wen Congyang

> 
>  1. If the fd is non-blocking, that means you don't want to block
>     and spinning for a long time will have the same effects
> 
>  2. Spinning consumes host resources
> 

Re: [Qemu-devel] [PATCH 2/3] qmp: dump-guest-memory: don't spin if non-blocking fd would block

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> At 09/24/2012 09:34 PM, Luiz Capitulino Wrote:
>> On Mon, 24 Sep 2012 14:27:17 +0800
>> Wen Congyang <wency@cn.fujitsu.com> wrote:
>> 
>>> At 09/22/2012 01:07 AM, Luiz Capitulino Wrote:
>>>> fd_write_vmcore() will indefinitely spin for a non-blocking
>>>> file-descriptor that would block. However, if the fd is non-blocking,
>>>> how does it make sense to spin?
>>>>
>>>> Change this behavior to return an error instead.
>>>>
>>>> Note that this can only happen with an fd provided by a management
>>>> application. The fd opened internally by dump-guest-memory is blocking.
>>>>
>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>>> ---
>>>>  dump.c | 13 +++----------
>>>>  1 file changed, 3 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/dump.c b/dump.c
>>>> index 2bf8d8d..5eea015 100644
>>>> --- a/dump.c
>>>> +++ b/dump.c
>>>> @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason)
>>>>  static int fd_write_vmcore(void *buf, size_t size, void *opaque)
>>>>  {
>>>>      DumpState *s = opaque;
>>>> -    int fd = s->fd;
>>>>      size_t writen_size;
>>>>  
>>>> -    /* The fd may be passed from user, and it can be non-blocked */
>>>> -    while (size) {
>>>> -        writen_size = qemu_write_full(fd, buf, size);
>>>> -        if (writen_size != size && errno != EAGAIN) {
>>>
>>> Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the
>>> function doesn't spin. What problems do you meet?
>> 
>> The problem is with non-blocking fds, where spinning isn't correct, for
>> two reasons:
>
> But, If the fd is non-blocking, errno can't be EAGAIN. So it doesn't spin.

I'm afraid you're confused.

http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

[EAGAIN] The O_NONBLOCK flag is set for the file descriptor and the
thread would be delayed in the write() operation.

Re: [Qemu-devel] [PATCH 2/3] qmp: dump-guest-memory: don't spin if non-blocking fd would block

[Wen Congyang]

At 09/25/2012 05:01 PM, Markus Armbruster Wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> At 09/24/2012 09:34 PM, Luiz Capitulino Wrote:
>>> On Mon, 24 Sep 2012 14:27:17 +0800
>>> Wen Congyang <wency@cn.fujitsu.com> wrote:
>>>
>>>> At 09/22/2012 01:07 AM, Luiz Capitulino Wrote:
>>>>> fd_write_vmcore() will indefinitely spin for a non-blocking
>>>>> file-descriptor that would block. However, if the fd is non-blocking,
>>>>> how does it make sense to spin?
>>>>>
>>>>> Change this behavior to return an error instead.
>>>>>
>>>>> Note that this can only happen with an fd provided by a management
>>>>> application. The fd opened internally by dump-guest-memory is blocking.
>>>>>
>>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>>>> ---
>>>>>  dump.c | 13 +++----------
>>>>>  1 file changed, 3 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/dump.c b/dump.c
>>>>> index 2bf8d8d..5eea015 100644
>>>>> --- a/dump.c
>>>>> +++ b/dump.c
>>>>> @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason)
>>>>>  static int fd_write_vmcore(void *buf, size_t size, void *opaque)
>>>>>  {
>>>>>      DumpState *s = opaque;
>>>>> -    int fd = s->fd;
>>>>>      size_t writen_size;
>>>>>  
>>>>> -    /* The fd may be passed from user, and it can be non-blocked */
>>>>> -    while (size) {
>>>>> -        writen_size = qemu_write_full(fd, buf, size);
>>>>> -        if (writen_size != size && errno != EAGAIN) {
>>>>
>>>> Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the
>>>> function doesn't spin. What problems do you meet?
>>>
>>> The problem is with non-blocking fds, where spinning isn't correct, for
>>> two reasons:
>>
>> But, If the fd is non-blocking, errno can't be EAGAIN. So it doesn't spin.
> 
> I'm afraid you're confused.
> 
> http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html
> 
> [EAGAIN] The O_NONBLOCK flag is set for the file descriptor and the
> thread would be delayed in the write() operation.
> 

Ahh, you are right.

Thanks
Wen Congyang

Re: [Qemu-devel] [PATCH 2/3] qmp: dump-guest-memory: don't spin if non-blocking fd would block

[Wen Congyang]

At 09/24/2012 09:34 PM, Luiz Capitulino Wrote:
> On Mon, 24 Sep 2012 14:27:17 +0800
> Wen Congyang <wency@cn.fujitsu.com> wrote:
> 
>> At 09/22/2012 01:07 AM, Luiz Capitulino Wrote:
>>> fd_write_vmcore() will indefinitely spin for a non-blocking
>>> file-descriptor that would block. However, if the fd is non-blocking,
>>> how does it make sense to spin?
>>>
>>> Change this behavior to return an error instead.
>>>
>>> Note that this can only happen with an fd provided by a management
>>> application. The fd opened internally by dump-guest-memory is blocking.
>>>
>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> ---
>>>  dump.c | 13 +++----------
>>>  1 file changed, 3 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/dump.c b/dump.c
>>> index 2bf8d8d..5eea015 100644
>>> --- a/dump.c
>>> +++ b/dump.c
>>> @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason)
>>>  static int fd_write_vmcore(void *buf, size_t size, void *opaque)
>>>  {
>>>      DumpState *s = opaque;
>>> -    int fd = s->fd;
>>>      size_t writen_size;
>>>  
>>> -    /* The fd may be passed from user, and it can be non-blocked */
>>> -    while (size) {
>>> -        writen_size = qemu_write_full(fd, buf, size);
>>> -        if (writen_size != size && errno != EAGAIN) {
>>
>> Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the
>> function doesn't spin. What problems do you meet?
> 
> The problem is with non-blocking fds, where spinning isn't correct, for
> two reasons:
> 
>  1. If the fd is non-blocking, that means you don't want to block
>     and spinning for a long time will have the same effects
> 
>  2. Spinning consumes host resources
> 


If so, I agree with it, and the patch looks fine to me

Thanks
Wen Congyang

[Qemu-devel] [PATCH 3/3] hmp: dump-guest-memory: hardcode protocol argument to file:

[Luiz Capitulino]

Today, it's necessary to specify the protocol you want to use
when dumping the guest memory, for example:

 (qemu) dump-guest-memory file:/tmp/guest-memory

This has a few issues:

 1. It's cumbersome to type
 2. We loose file path autocompletion
 3. Being able to specify fd:X in HMP makes little sense for humans

Because of these reasons, hardcode the 'protocol' argument to
'file:' in HMP.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx |  8 +++-----
 hmp.c           | 11 ++++++++---
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index ed67e99..0302458 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -914,12 +914,11 @@ ETEXI
 #if defined(CONFIG_HAVE_CORE_DUMP)
     {
         .name       = "dump-guest-memory",
-        .args_type  = "paging:-p,protocol:s,begin:i?,length:i?",
-        .params     = "[-p] protocol [begin] [length]",
+        .args_type  = "paging:-p,filename:F,begin:i?,length:i?",
+        .params     = "[-p] filename [begin] [length]",
         .help       = "dump guest memory to file"
                       "\n\t\t\t begin(optional): the starting physical address"
                       "\n\t\t\t length(optional): the memory size, in bytes",
-        .user_print = monitor_user_noop,
         .mhandler.cmd = hmp_dump_guest_memory,
     },
 
@@ -929,8 +928,7 @@ STEXI
 @findex dump-guest-memory
 Dump guest memory to @var{protocol}. The file can be processed with crash or
 gdb.
-  protocol: destination file(started with "file:") or destination file
-            descriptor (started with "fd:")
+  filename: dump file name
     paging: do paging to get guest's memory mapping
      begin: the starting physical address. It's optional, and should be
             specified with length together.
diff --git a/hmp.c b/hmp.c
index ba6fbd3..513b40b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1042,11 +1042,12 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
 {
     Error *errp = NULL;
     int paging = qdict_get_try_bool(qdict, "paging", 0);
-    const char *file = qdict_get_str(qdict, "protocol");
+    const char *file = qdict_get_str(qdict, "filename");
     bool has_begin = qdict_haskey(qdict, "begin");
     bool has_length = qdict_haskey(qdict, "length");
     int64_t begin = 0;
     int64_t length = 0;
+    QString *prot;
 
     if (has_begin) {
         begin = qdict_get_int(qdict, "begin");
@@ -1055,9 +1056,13 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
         length = qdict_get_int(qdict, "length");
     }
 
-    qmp_dump_guest_memory(paging, file, has_begin, begin, has_length, length,
-                          &errp);
+    prot = qstring_from_str("file:");
+    qstring_append(prot, file);
+
+    qmp_dump_guest_memory(paging, qstring_get_str(prot), has_begin, begin,
+                          has_length, length, &errp);
     hmp_handle_error(mon, &errp);
+    QDECREF(prot);
 }
 
 void hmp_netdev_add(Monitor *mon, const QDict *qdict)
-- 
1.7.12.315.g682ce8b

Re: [Qemu-devel] [PATCH 3/3] hmp: dump-guest-memory: hardcode protocol argument to file:

[Eric Blake]

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

On 09/21/2012 11:07 AM, Luiz Capitulino wrote:
> Today, it's necessary to specify the protocol you want to use
> when dumping the guest memory, for example:
> 
>  (qemu) dump-guest-memory file:/tmp/guest-memory
> 
> This has a few issues:
> 
>  1. It's cumbersome to type
>  2. We loose file path autocompletion
>  3. Being able to specify fd:X in HMP makes little sense for humans
> 
> Because of these reasons, hardcode the 'protocol' argument to
> 'file:' in HMP.

No impact to libvirt as a QMP client, and we've already declared that
HMP can change for better UI even if it is not back-compatible.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 3/3] hmp: dump-guest-memory: hardcode protocol argument to file:

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Today, it's necessary to specify the protocol you want to use
> when dumping the guest memory, for example:
>
>  (qemu) dump-guest-memory file:/tmp/guest-memory
>
> This has a few issues:
>
>  1. It's cumbersome to type
>  2. We loose file path autocompletion
>  3. Being able to specify fd:X in HMP makes little sense for humans
>
> Because of these reasons, hardcode the 'protocol' argument to
> 'file:' in HMP.

No objection.

The QMP wart remains: protocol is a string that needs to be parsed.  I
feel that should be avoided in QMP.  We should have expressed "either
filename or file descriptor name" in the schema.  Let's avoid such
mistakes in the future.

One remark inline.

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hmp-commands.hx |  8 +++-----
>  hmp.c           | 11 ++++++++---
>  2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index ed67e99..0302458 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -914,12 +914,11 @@ ETEXI
>  #if defined(CONFIG_HAVE_CORE_DUMP)
>      {
>          .name       = "dump-guest-memory",
> -        .args_type  = "paging:-p,protocol:s,begin:i?,length:i?",
> -        .params     = "[-p] protocol [begin] [length]",
> +        .args_type  = "paging:-p,filename:F,begin:i?,length:i?",
> +        .params     = "[-p] filename [begin] [length]",
>          .help       = "dump guest memory to file"
>                        "\n\t\t\t begin(optional): the starting physical address"
>                        "\n\t\t\t length(optional): the memory size, in bytes",
> -        .user_print = monitor_user_noop,
>          .mhandler.cmd = hmp_dump_guest_memory,
>      },
>  
> @@ -929,8 +928,7 @@ STEXI
>  @findex dump-guest-memory
>  Dump guest memory to @var{protocol}. The file can be processed with crash or
>  gdb.
> -  protocol: destination file(started with "file:") or destination file
> -            descriptor (started with "fd:")
> +  filename: dump file name
>      paging: do paging to get guest's memory mapping
>       begin: the starting physical address. It's optional, and should be
>              specified with length together.
> diff --git a/hmp.c b/hmp.c
> index ba6fbd3..513b40b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1042,11 +1042,12 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>  {
>      Error *errp = NULL;
>      int paging = qdict_get_try_bool(qdict, "paging", 0);
> -    const char *file = qdict_get_str(qdict, "protocol");
> +    const char *file = qdict_get_str(qdict, "filename");
>      bool has_begin = qdict_haskey(qdict, "begin");
>      bool has_length = qdict_haskey(qdict, "length");
>      int64_t begin = 0;
>      int64_t length = 0;
> +    QString *prot;
>  
>      if (has_begin) {
>          begin = qdict_get_int(qdict, "begin");
> @@ -1055,9 +1056,13 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>          length = qdict_get_int(qdict, "length");
>      }
>  
> -    qmp_dump_guest_memory(paging, file, has_begin, begin, has_length, length,
> -                          &errp);
> +    prot = qstring_from_str("file:");
> +    qstring_append(prot, file);

What about prot = g_strconcat("file:", file, NULL) ... g_free(prot) ?

> +
> +    qmp_dump_guest_memory(paging, qstring_get_str(prot), has_begin, begin,
> +                          has_length, length, &errp);
>      hmp_handle_error(mon, &errp);
> +    QDECREF(prot);
>  }
>  
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict)

Re: [Qemu-devel] [PATCH 3/3] hmp: dump-guest-memory: hardcode protocol argument to file:

[Luiz Capitulino]

On Tue, 25 Sep 2012 10:48:05 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Today, it's necessary to specify the protocol you want to use
> > when dumping the guest memory, for example:
> >
> >  (qemu) dump-guest-memory file:/tmp/guest-memory
> >
> > This has a few issues:
> >
> >  1. It's cumbersome to type
> >  2. We loose file path autocompletion
> >  3. Being able to specify fd:X in HMP makes little sense for humans
> >
> > Because of these reasons, hardcode the 'protocol' argument to
> > 'file:' in HMP.
> 
> No objection.
> 
> The QMP wart remains: protocol is a string that needs to be parsed.  I
> feel that should be avoided in QMP.  We should have expressed "either
> filename or file descriptor name" in the schema.  Let's avoid such
> mistakes in the future.

I think that the right solution would be to have an URI.

> One remark inline.
> 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hmp-commands.hx |  8 +++-----
> >  hmp.c           | 11 ++++++++---
> >  2 files changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index ed67e99..0302458 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -914,12 +914,11 @@ ETEXI
> >  #if defined(CONFIG_HAVE_CORE_DUMP)
> >      {
> >          .name       = "dump-guest-memory",
> > -        .args_type  = "paging:-p,protocol:s,begin:i?,length:i?",
> > -        .params     = "[-p] protocol [begin] [length]",
> > +        .args_type  = "paging:-p,filename:F,begin:i?,length:i?",
> > +        .params     = "[-p] filename [begin] [length]",
> >          .help       = "dump guest memory to file"
> >                        "\n\t\t\t begin(optional): the starting physical address"
> >                        "\n\t\t\t length(optional): the memory size, in bytes",
> > -        .user_print = monitor_user_noop,
> >          .mhandler.cmd = hmp_dump_guest_memory,
> >      },
> >  
> > @@ -929,8 +928,7 @@ STEXI
> >  @findex dump-guest-memory
> >  Dump guest memory to @var{protocol}. The file can be processed with crash or
> >  gdb.
> > -  protocol: destination file(started with "file:") or destination file
> > -            descriptor (started with "fd:")
> > +  filename: dump file name
> >      paging: do paging to get guest's memory mapping
> >       begin: the starting physical address. It's optional, and should be
> >              specified with length together.
> > diff --git a/hmp.c b/hmp.c
> > index ba6fbd3..513b40b 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1042,11 +1042,12 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> >  {
> >      Error *errp = NULL;
> >      int paging = qdict_get_try_bool(qdict, "paging", 0);
> > -    const char *file = qdict_get_str(qdict, "protocol");
> > +    const char *file = qdict_get_str(qdict, "filename");
> >      bool has_begin = qdict_haskey(qdict, "begin");
> >      bool has_length = qdict_haskey(qdict, "length");
> >      int64_t begin = 0;
> >      int64_t length = 0;
> > +    QString *prot;
> >  
> >      if (has_begin) {
> >          begin = qdict_get_int(qdict, "begin");
> > @@ -1055,9 +1056,13 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> >          length = qdict_get_int(qdict, "length");
> >      }
> >  
> > -    qmp_dump_guest_memory(paging, file, has_begin, begin, has_length, length,
> > -                          &errp);
> > +    prot = qstring_from_str("file:");
> > +    qstring_append(prot, file);
> 
> What about prot = g_strconcat("file:", file, NULL) ... g_free(prot) ?

Good suggestion.

> 
> > +
> > +    qmp_dump_guest_memory(paging, qstring_get_str(prot), has_begin, begin,
> > +                          has_length, length, &errp);
> >      hmp_handle_error(mon, &errp);
> > +    QDECREF(prot);
> >  }
> >  
> >  void hmp_netdev_add(Monitor *mon, const QDict *qdict)
> 

Re: [Qemu-devel] [PATCH 3/3] hmp: dump-guest-memory: hardcode protocol argument to file:

[Wen Congyang]

At 09/22/2012 01:07 AM, Luiz Capitulino Wrote:
> Today, it's necessary to specify the protocol you want to use
> when dumping the guest memory, for example:
> 
>  (qemu) dump-guest-memory file:/tmp/guest-memory
> 
> This has a few issues:
> 
>  1. It's cumbersome to type
>  2. We loose file path autocompletion
>  3. Being able to specify fd:X in HMP makes little sense for humans
> 
> Because of these reasons, hardcode the 'protocol' argument to
> 'file:' in HMP.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

This patch looks fine to me.

Thanks
Wen Congyang

> ---
>  hmp-commands.hx |  8 +++-----
>  hmp.c           | 11 ++++++++---
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index ed67e99..0302458 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -914,12 +914,11 @@ ETEXI
>  #if defined(CONFIG_HAVE_CORE_DUMP)
>      {
>          .name       = "dump-guest-memory",
> -        .args_type  = "paging:-p,protocol:s,begin:i?,length:i?",
> -        .params     = "[-p] protocol [begin] [length]",
> +        .args_type  = "paging:-p,filename:F,begin:i?,length:i?",
> +        .params     = "[-p] filename [begin] [length]",
>          .help       = "dump guest memory to file"
>                        "\n\t\t\t begin(optional): the starting physical address"
>                        "\n\t\t\t length(optional): the memory size, in bytes",
> -        .user_print = monitor_user_noop,
>          .mhandler.cmd = hmp_dump_guest_memory,
>      },
>  
> @@ -929,8 +928,7 @@ STEXI
>  @findex dump-guest-memory
>  Dump guest memory to @var{protocol}. The file can be processed with crash or
>  gdb.
> -  protocol: destination file(started with "file:") or destination file
> -            descriptor (started with "fd:")
> +  filename: dump file name
>      paging: do paging to get guest's memory mapping
>       begin: the starting physical address. It's optional, and should be
>              specified with length together.
> diff --git a/hmp.c b/hmp.c
> index ba6fbd3..513b40b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1042,11 +1042,12 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>  {
>      Error *errp = NULL;
>      int paging = qdict_get_try_bool(qdict, "paging", 0);
> -    const char *file = qdict_get_str(qdict, "protocol");
> +    const char *file = qdict_get_str(qdict, "filename");
>      bool has_begin = qdict_haskey(qdict, "begin");
>      bool has_length = qdict_haskey(qdict, "length");
>      int64_t begin = 0;
>      int64_t length = 0;
> +    QString *prot;
>  
>      if (has_begin) {
>          begin = qdict_get_int(qdict, "begin");
> @@ -1055,9 +1056,13 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>          length = qdict_get_int(qdict, "length");
>      }
>  
> -    qmp_dump_guest_memory(paging, file, has_begin, begin, has_length, length,
> -                          &errp);
> +    prot = qstring_from_str("file:");
> +    qstring_append(prot, file);
> +
> +    qmp_dump_guest_memory(paging, qstring_get_str(prot), has_begin, begin,
> +                          has_length, length, &errp);
>      hmp_handle_error(mon, &errp);
> +    QDECREF(prot);
>  }
>  
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict)

Re: [Qemu-devel] [PATCH 0/3]: qmp/hmp: dump-guest-memory fixes

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Please see individual patches for details.

Looks good to me, except for the tab damage Eric noted.  The
simplification I suggested for 3/3 is optional; use it if you like it.

[Qemu-devel] [PATCH v2 0/3] qmp/hmp: dump-guest-memory fixes

[Luiz Capitulino]

Please, check individual patches for details.

v2

 - Fix TAB/spaces in qapi-schema.json
 - English fixes
 - Use g_strconcat() (instead of a qstring)

Luiz Capitulino (3):
  qmp: dump-guest-memory: improve schema doc (again)
  qmp: dump-guest-memory: don't spin if non-blocking fd would block
  hmp: dump-guest-memory: hardcode protocol argument to "file:"

 dump.c           | 15 ++++-----------
 hmp-commands.hx  |  8 +++-----
 hmp.c            |  8 ++++++--
 qapi-schema.json | 32 ++++++++++++++++++++------------
 4 files changed, 33 insertions(+), 30 deletions(-)

-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 2/3] qmp: dump-guest-memory: don't spin if non-blocking fd would block

[Luiz Capitulino]

fd_write_vmcore() will indefinitely spin for a non-blocking
file-descriptor that would block. However, if the fd is non-blocking,
how does it make sense to spin?

Change this behavior to return an error instead.

Note that this can only happen with an fd provided by a management
application. The fd opened internally by dump-guest-memory is blocking.

While there, also fix 'writen_size' variable name.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 dump.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/dump.c b/dump.c
index 2bf8d8d..81c3624 100644
--- a/dump.c
+++ b/dump.c
@@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason)
 static int fd_write_vmcore(void *buf, size_t size, void *opaque)
 {
     DumpState *s = opaque;
-    int fd = s->fd;
-    size_t writen_size;
+    size_t written_size;
 
-    /* The fd may be passed from user, and it can be non-blocked */
-    while (size) {
-        writen_size = qemu_write_full(fd, buf, size);
-        if (writen_size != size && errno != EAGAIN) {
-            return -1;
-        }
-
-        buf += writen_size;
-        size -= writen_size;
+    written_size = qemu_write_full(s->fd, buf, size);
+    if (written_size != size) {
+        return -1;
     }
 
     return 0;
-- 
1.7.12.315.g682ce8b