On Thu, Jun 05, 2008 at 10:30:55PM +0100, Richard W.M. Jones wrote:

+/* Memory peeking flags. */ +typedef enum { + VIR_MEMORY_VIRTUAL = 1, /* addresses are virtual addresses */ +} virDomainMemoryFlags;

Since there is only one flag, and it is compulsory I'm rather inclined to say that virtual memory addressing should be the default with a flags value of 0. Unless there is another mode, not yet implemented, that you think would be a better default in the future ? Obviously keep the flags arg for expansion regardless though.

diff -u -p -r1.36 remote.c --- qemud/remote.c 5 Jun 2008 21:12:26 -0000 1.36 +++ qemud/remote.c 5 Jun 2008 21:26:29 -0000 @@ -938,6 +938,52 @@ remoteDispatchDomainBlockPeek (struct qe }

static int +remoteDispatchDomainMemoryPeek (struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client, + remote_message_header *req, + remote_domain_memory_peek_args *args, + remote_domain_memory_peek_ret *ret) +{ + virDomainPtr dom; + unsigned long long offset; + size_t size; + unsigned int flags; + CHECK_CONN (client); + + dom = get_nonnull_domain (client->conn, args->dom); + if (dom == NULL) { + remoteDispatchError (client, req, "%s", _("domain not found")); + return -2; + } + offset = args->offset; + size = args->size; + flags = args->flags; + + if (size > REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX) { + remoteDispatchError (client, req, + "%s", _("size > maximum buffer size")); + return -2; + } + + ret->buffer.buffer_len = size; + ret->buffer.buffer_val = malloc (size); + if (!ret->buffer.buffer_val) { + remoteDispatchError (client, req, "%s", strerror (errno)); + return -2; + }

The 'dom' object is leaking in the 2 error paths here & it'd be better to use VIR_ALLOC_N(ret->buffer.buffer_val, size) here.

diff -u -p -r1.14 remote_protocol.x --- qemud/remote_protocol.x 5 Jun 2008 21:12:27 -0000 1.14 +++ qemud/remote_protocol.x 5 Jun 2008 21:26:29 -0000 @@ -340,6 +340,17 @@ struct remote_domain_block_peek_ret { opaque buffer<REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX>; };

+struct remote_domain_memory_peek_args { + remote_nonnull_domain dom; + unsigned hyper offset; + unsigned size; + unsigned flags; +}; + +struct remote_domain_memory_peek_ret { + opaque buffer<REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX>; +};

Is it worth having a separate constant for the max memory size, independant of the block peek size.

+/** + * virDomainMemoryPeek: + * @dom: pointer to the domain object + * @start: start of memory to peek + * @size: size of memory to peek + * @buffer: return buffer (must be at least size bytes) + * @flags: flags, see below + * + * This function allows you to read the contents of a domain's + * memory. + * + * The memory which is read is controlled by the 'start', 'size' + * and 'flags' parameters. + * + * If 'flags' is VIR_MEMORY_VIRTUAL then the 'start' and 'size' + * parameters are interpreted as virtual memory addresses for + * whichever task happens to be running on the domain at the + * moment. Although this sounds haphazard it is in fact what + * you want in order to read Linux kernel state, because it + * ensures that pointers in the kernel image can be interpreted + * coherently. + * + * 'buffer' is the return buffer and must be at least 'size' bytes. + * 'size' may be 0 to test if the call would succeed.

Should we document and enforce a maximum value for size. The remote driver at least has a maximum limit,, so perhaps we should enforce that in the main API dispatcher here in virDomainMemoryPeek() impl

+int +static int +qemudDomainMemoryPeek (virDomainPtr dom, + unsigned long long offset, size_t size, + void *buffer, + unsigned int flags) +{ + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + struct qemud_vm *vm = qemudFindVMByID (driver, dom->id); + char cmd[256], *info; + char tmp[] = "/tmp/qemumemXXXXXX";

It'd make the SELinux policy easier if we avoided the generic /tmp and put the files in somewhere like /var/spool/libvirt/qemu/. Wouldn't even need to use mkstemp() then - just have it named VMNAME.mem since it'd be in a safe controlled directory Regards, Daniel. -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|