[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Support physical memory in virDomainMemoryPeek()



On Wed, Jul 22, 2009 at 12:36:10PM +0900, Nguyen Anh Quynh wrote:
> Hi,
> 
> This patch provides support for physical memory in
> virDomainMemoryPeek(). Please consider applying.
> 
> Signed-off-by: Nguyen Anh Quynh <aquynh gmail com>

  okay a couple of comments:

> # diffstat pmemsave3.diff
>  docs/libvirt-api.xml         |    2 +-
>  include/libvirt/libvirt.h.in |    1 +
>  src/libvirt.c                |   14 +++++---------
>  src/qemu_driver.c            |   17 ++++++++---------
>  4 files changed, 15 insertions(+), 19 deletions(-)

> diff --git a/docs/libvirt-api.xml b/docs/libvirt-api.xml
> index 8ded57a..04b1108 100644
> --- a/docs/libvirt-api.xml
> +++ b/docs/libvirt-api.xml

  This is generated, once the comment in libvirt.c is updated this is
extracted automatically (cd docs ; make rebuild), no need to add it to
the patch

> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index ba2b6f0..e6536c7 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -619,6 +619,7 @@ int                     virDomainBlockPeek (virDomainPtr dom,
>  /* Memory peeking flags. */
>  typedef enum {
>    VIR_MEMORY_VIRTUAL              = 1, /* addresses are virtual addresses */
> +  VIR_MEMORY_PHYSICAL             = 2, /* addresses are physical addresses */
>  } virDomainMemoryFlags;
>  
>  int                     virDomainMemoryPeek (virDomainPtr dom,
> diff --git a/src/libvirt.c b/src/libvirt.c
> index f4a7fa7..393d0c1 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -3815,19 +3815,20 @@ virDomainMemoryPeek (virDomainPtr dom,
>          goto error;
>      }
>  
> -    /* Flags must be VIR_MEMORY_VIRTUAL at the moment.
> -     *
> -     * Note on access to physical memory: A VIR_MEMORY_PHYSICAL flag is
> +    /* Note on access to physical memory: A VIR_MEMORY_PHYSICAL flag is
>       * a possibility.  However it isn't really useful unless the caller
>       * can also access registers, particularly CR3 on x86 in order to
>       * get the Page Table Directory.  Since registers are different on
>       * every architecture, that would imply another call to get the
>       * machine registers.
>       *
> -     * The QEMU driver handles only VIR_MEMORY_VIRTUAL, mapping it
> +     * The QEMU driver handles VIR_MEMORY_VIRTUAL, mapping it
>       * to the qemu 'memsave' command which does the virtual to physical
>       * mapping inside qemu.
>       *
> +     * The QEMU driver also handles VIR_MEMORY_PHYSICAL, mapping it
> +     * to the qemu 'pmemsave' command.
> +     *
>       * At time of writing there is no Xen driver.  However the Xen
>       * hypervisor only lets you map physical pages from other domains,
>       * and so the Xen driver would have to do the virtual to physical

  Okay

> @@ -3836,11 +3837,6 @@ virDomainMemoryPeek (virDomainPtr dom,
>       * which does this, although we cannot copy this code directly
>       * because of incompatible licensing.
>       */
> -    if (flags != VIR_MEMORY_VIRTUAL) {
> -        virLibDomainError (dom, VIR_ERR_INVALID_ARG,
> -                           _("flags parameter must be VIR_MEMORY_VIRTUAL"));
> -        goto error;
> -    }

  The check should be preserved as
    if ((flags != VIR_MEMORY_VIRTUAL) && (flags != VIR_MEMORY_PHYSICAL)) {
and update the error message

>      /* Allow size == 0 as an access test. */
>      if (size > 0 && !buffer) {
> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
> index 00dc6e5..995cbee 100644
> --- a/src/qemu_driver.c
> +++ b/src/qemu_driver.c
> @@ -5181,12 +5181,6 @@ qemudDomainMemoryPeek (virDomainPtr dom,
>          goto cleanup;
>      }
>  
> -    if (flags != VIR_MEMORY_VIRTUAL) {
> -        qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
> -                          "%s", _("QEMU driver only supports virtual memory addrs"));
> -        goto cleanup;
> -    }
> -

  I tend to think the check should be modified similary here

>      if (!virDomainIsActive(vm)) {
>          qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_INVALID,
>                           "%s", _("domain is not running"));
> @@ -5200,15 +5194,20 @@ qemudDomainMemoryPeek (virDomainPtr dom,
>          goto cleanup;
>      }
>  
> -    /* Issue the memsave command. */
> -    snprintf (cmd, sizeof cmd, "memsave %llu %zi \"%s\"", offset, size, tmp);
> +    if (flags == VIR_MEMORY_VIRTUAL)
> +        /* Issue the memsave command. */
> +        snprintf (cmd, sizeof cmd, "memsave %llu %zi \"%s\"", offset, size, tmp);
> +    else
> +        /* flags == VIR_MEMORY_PHYSICAL, issue the pmemsave command. */
> +        snprintf (cmd, sizeof cmd, "pmemsave %llu %zi \"%s\"", offset, size, tmp);
> +

  Then having no error handling here would make sense, but currently
if you pass a wrong argument you would just silently assume flags ==
VIR_MEMORY_PHYSICAL, so the patch as-is is not correct.

>      if (qemudMonitorCommand (vm, cmd, &info) < 0) {
>          qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
>                            "%s", _("'memsave' command failed"));
>          goto cleanup;
>      }
>  
> -    DEBUG ("%s: memsave reply: %s", vm->def->name, info);
> +    DEBUG ("%s: (p)memsave reply: %s", vm->def->name, info);
>  
>      /* Read the memory file into buffer. */
>      if (saferead (fd, buffer, size) == (ssize_t) -1) {

  thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]