On 10/15/2015 05:44 PM, vale...@aimale.com wrote: > From: Valerio Aimale <vale...@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 (bdpa...@acm.org)
> + *
> + * Refurbished for modern QEMU by Valerio Aimale (vale...@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
signature.asc
Description: OpenPGP digital signature
