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/