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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list