Re: [Qemu-devel] [PATCH] QEMU patch for libvmi to introspect QEMU/kvm virtual machines. Usually this patch is distributed with libvmi, but, it might be more useful to have it in the QEMU source permanently.

[Eric Blake <eblake@redhat.com>]

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

On 10/15/2015 05:44 PM, valerio@aimale.com wrote:
> From: Valerio Aimale <valerio@aimale.com>

Long subject line, and no message body.  Remember, you want the subject
line to be a one-line short summary of 'what', then the commit body
message for 'why', as in:

qmp: add command for libvmi memory introspection

In the past, libvmi was relying on an out-of-tree patch to qemu that
provides a new QMP command pmemaccess.  It is now time to make this
command part of qemu.

pmemaccess is used to create a side-channel communication path that can
more effectively be used to query lots of small memory chunks without
the overhead of one QMP command per chunk. ...

> 
> ---

You are missing a Signed-off-by: tag.  Without that, we cannot take your
patch.  But at least we can still review it:

>  Makefile.target  |   2 +-
>  hmp-commands.hx  |  14 ++++
>  hmp.c            |   9 +++
>  hmp.h            |   1 +
>  memory-access.c  | 206 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  memory-access.h  |  21 ++++++
>  qapi-schema.json |  28 ++++++++
>  qmp-commands.hx  |  23 +++++++
>  8 files changed, 303 insertions(+), 1 deletion(-)
>  create mode 100644 memory-access.c
>  create mode 100644 memory-access.h
> 
> diff --git a/Makefile.target b/Makefile.target
> index 962d004..940ab51 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -131,7 +131,7 @@ endif #CONFIG_BSD_USER
>  #########################################################
>  # System emulator target
>  ifdef CONFIG_SOFTMMU
> -obj-y += arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o
> +obj-y += arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o memory-access.o

This line is now over 80 columns; please wrap.

>  obj-y += qtest.o bootdevice.o

In fact, you could have just appended it into this line instead.

> +++ b/hmp-commands.hx
> @@ -807,6 +807,20 @@ save to disk physical memory dump starting at @var{addr} of size @var{size}.
>  ETEXI
>  
>      {
> +        .name       = "pmemaccess",
> +        .args_type  = "path:s",
> +        .params     = "file",
> +        .help       = "open A UNIX Socket access to physical memory at 'path'",

s/A/a/

Awkward grammar; better might be:

open a Unix socket at 'path' for use in accessing physical memory

Please also document where the user can find the protocol that will be
used across the side-channel socket thus opened.


> +++ b/memory-access.c
> @@ -0,0 +1,206 @@
> +/*
> + * Access guest physical memory via a domain socket.
> + *
> + * Copyright (C) 2011 Sandia National Laboratories
> + * Original Author: Bryan D. Payne (bdpayne@acm.org)
> + * 
> + * Refurbished for modern QEMU by Valerio Aimale (valerio@aimale.com), in 2015
> + */

I would expect at least something under docs/ in addition to this file
(the protocol spoken over the socket should be well-documented, and not
just by reading the source code).  Compare with docs/qmp-spec.txt.

> +struct request{
> +    uint8_t type;      // 0 quit, 1 read, 2 write, ... rest reserved
> +    uint64_t address;  // address to read from OR write to
> +    uint64_t length;   // number of bytes to read OR write

Any particular endianness constraints to worry about?

> +};
> +
> +static uint64_t
> +connection_read_memory (uint64_t user_paddr, void *buf, uint64_t user_len)
> +{
> +    hwaddr paddr = (hwaddr) user_paddr;
> +    hwaddr len = (hwaddr) user_len;
> +    void *guestmem = cpu_physical_memory_map(paddr, &len, 0);
> +    if (!guestmem){

Space before {, throughout the patch.

> +static void
> +send_success_ack (int connection_fd)

No space before ( in function usage.

> +{
> +    uint8_t success = 1;

Magic number; I'd expect an enum or #defines somewhere.

> +    int nbytes = write(connection_fd, &success, 1);
> +    if (1 != nbytes){
> +        fprintf(stderr, "Qemu pmemaccess: failed to send success ack\n");

Is fprintf() really the best approach for error reporting?


> +static void
> +connection_handler (int connection_fd)
> +{
> +    int nbytes;
> +    struct request req;
> +
> +    while (1){
> +        // client request should match the struct request format

We prefer /* */ comments over //.

> +        nbytes = read(connection_fd, &req, sizeof(struct request));
> +        if (nbytes != sizeof(struct request)){
> +            // error
> +            continue;

Silently ignoring errors?

> +        }
> +        else if (req.type == 0){
> +            // request to quit, goodbye
> +            break;
> +        }
> +        else if (req.type == 1){
> +            // request to read
> +            char *buf = malloc(req.length + 1);

Mixing g_malloc/g_free with malloc/free makes it a bit harder to reason
about things; it might be better to use only glib allocation.

> +            nbytes = connection_read_memory(req.address, buf, req.length);
> +            if (nbytes != req.length){
> +                // read failure, return failure message
> +                buf[req.length] = 0; // set last byte to 0 for failure
> +                nbytes = write(connection_fd, buf, 1);
> +            }
> +            else{
> +                // read success, return bytes
> +                buf[req.length] = 1; // set last byte to 1 for success
> +                nbytes = write(connection_fd, buf, nbytes + 1);
> +            }
> +            free(buf);
> +        }
> +        else if (req.type == 2){
> +            // request to write
> +            void *write_buf = malloc(req.length);
> +            nbytes = read(connection_fd, &write_buf, req.length);

No error checking that the allocation succeeded? How do you protect from
bogus requests?

> +            if (nbytes != req.length){
> +                // failed reading the message to write
> +                send_fail_ack(connection_fd);
> +            }
> +            else{

} on the same line as else

> +                // do the write
> +                nbytes = connection_write_memory(req.address, write_buf, req.length);
> +                if (nbytes == req.length){
> +                    send_success_ack(connection_fd);
> +                }
> +                else{
> +                    send_fail_ack(connection_fd);
> +                }
> +            }
> +            free(write_buf);
> +        }
> +        else{
> +            // unknown command
> +            fprintf(stderr, "Qemu pmemaccess: ignoring unknown command (%d)\n", req.type);
> +            char *buf = malloc(1);
> +            buf[0] = 0;
> +            nbytes = write(connection_fd, buf, 1);
> +            free(buf);
> +        }
> +    }
> +
> +    close(connection_fd);
> +}
> +
> +static void *
> +memory_access_thread (void *p)

The most common style in qemu puts return type on the same line as the
function name.

> +{
> +    struct sockaddr_un address;
> +    int socket_fd, connection_fd;
> +    socklen_t address_length;
> +    struct pmemaccess_args *pargs = (struct pmemaccess_args *)p;

This cast is not necessary in C.

> +
> +    socket_fd = socket(PF_UNIX, SOCK_STREAM, 0);
> +    if (socket_fd < 0){
> +	error_setg(pargs->errp, "Qemu pmemaccess: socket failed");

TAB damage.  Also, mentioning 'Qemu' in an error message is probably
redundant.  That, and we prefer 'qemu' over 'Qemu'.

> +        goto error_exit;
> +    }
> +    unlink(pargs->path);

No check for errors?

> +    address.sun_family = AF_UNIX;
> +    address_length = sizeof(address.sun_family) + sprintf(address.sun_path, "%s", (char *) pargs->path);

Long line.  sprintf() is dangerous if you are not positive that
pargs->path fits.  Why do you need the cast?

> +
> +    if (bind(socket_fd, (struct sockaddr *) &address, address_length) != 0){
> +	error_setg(pargs->errp, "Qemu pmemaccess: bind failed");

More TAB damage.  Please read
http://wiki.qemu.org/Contribute/SubmitAPatch for more contribution
hints, and be sure to run ./scripts/checkpatch.pl.


> +void
> +qmp_pmemaccess (const char *path, Error **errp)
> +{
> +    pthread_t thread;
> +    sigset_t set, oldset;
> +    struct pmemaccess_args *pargs;
> +
> +    // create the args struct
> +    pargs = (struct pmemaccess_args *) malloc(sizeof(struct pmemaccess_args));
> +    pargs->errp = errp;
> +    // create a copy of path that we can safely use
> +    pargs->path = malloc(strlen(path) + 1);
> +    memcpy(pargs->path, path, strlen(path) + 1);

g_strdup() is your friend.


> +++ b/qapi-schema.json
> @@ -1427,6 +1427,34 @@
>    'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
>  
>  ##
> +# @pmemaccess:
> +#
> +# Open A UNIX Socket access to physical memory

s/A UNIX Socket/a Unix socket/

Same wording suggestion as earlier; might be better as:

Open a Unix socket used as a side-channel for efficiently accessing
physical memory.

> +#
> +# @path: the name of the UNIX socket pipe
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 2.4.0.1

2.5, not 2.4.0.1.

> +#
> +# Notes: Derived from previously existing patches.

Dead sentence that doesn't add anything to the current specification.

> When command
> +# succeeds connect to the open socket. Write a binary structure to 
> +# the socket as:
> +# 
> +# struct request {
> +#     uint8_t type;   // 0 quit, 1 read, 2 write, ... rest reserved
> +#     uint64_t address;   // address to read from OR write to
> +#     uint64_t length;    // number of bytes to read OR write
> +# };
> +# 
> +# If it is a read operation, Qemu will return lenght+1 bytes. Read lenght+1

s/lenght/length/ twice

> +# bytes. the last byte will be a 1 for success, or a 0 for failure.
> +#  
> +##
> +{ 'command': 'pmemaccess',
> +  'data': {'path': 'str'} }
> +
> +##
>  # @cont:
>  #
>  # Resume guest VCPU execution.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index d2ba800..26e4a51 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -472,6 +472,29 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "pmemaccess",
> +        .args_type  = "path:s",
> +        .mhandler.cmd_new = qmp_marshal_input_pmemaccess,
> +    },
> +
> +SQMP
> +pmemaccess
> +----------
> +
> +Open A UNIX Socket access to physical memory
> +
> +Arguments:
> +
> +- "path": mount point path (json-string)
> +
> +Example:
> +
> +-> { "execute": "pmemaccess",
> +             "arguments": { "path": "/tmp/guestname" } }
> +<- { "return": {} }
> +
> +EQMP
> +    {
>          .name       = "inject-nmi",
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_inject_nmi,
> 

-- 
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: 604 bytes --]