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@xxxxxxxxx>
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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list