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(a)gmail.com&gt;  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.