[libvirt] [PATCH] Support physical memory in virDomainMemoryPeek()
Hi, This patch provides support for physical memory in virDomainMemoryPeek(). Please consider applying. Signed-off-by: Nguyen Anh Quynh <aquynh@gmail.com> # 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(-)
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/
On Wed, Jul 22, 2009 at 4:02 PM, Daniel Veillard<veillard@redhat.com> wrote:
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
Yes, this makes sense.
/* 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.
Actually I dont think it is necessary to do sanity check inside the driver, as it is already checked outside, inside virDomainMemoryPeek() (I will fix the patch, as agreed above). So do you really want to check it again here? Thanks, Quynh
On Wed, Jul 22, 2009 at 04:19:48PM +0900, Nguyen Anh Quynh wrote:
On Wed, Jul 22, 2009 at 4:02 PM, Daniel Veillard<veillard@redhat.com> wrote: [...]
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.
Actually I dont think it is necessary to do sanity check inside the driver, as it is already checked outside, inside virDomainMemoryPeek() (I will fix the patch, as agreed above).
So do you really want to check it again here?
yes because a driver may support only a subset of the flags allowed by the API, it's a bit redundant but it's the status quo, and I think it makes sense. 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/
On Wed, Jul 22, 2009 at 4:19 PM, Nguyen Anh Quynh<aquynh@gmail.com> wrote:
On Wed, Jul 22, 2009 at 4:02 PM, Daniel Veillard<veillard@redhat.com> wrote:
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
Yes, this makes sense.
/* 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.
Actually I dont think it is necessary to do sanity check inside the driver, as it is already checked outside, inside virDomainMemoryPeek() (I will fix the patch, as agreed above).
So do you really want to check it again here?
Acutally, to avoid all those ugly sanity checks, it is best to define VIR_MEMORY_* as an enum type, then redefine virDomainMemoryCheck() as (note the last param is changed): int virDomainMemoryPeek (virDomainPtr dom, unsigned long long start, size_t size, void *buffer, enum virDomainMemoryFlags flags); I didnt do that in my patch because that might cause some incompatibility to those applications using this function. But technically, that is nicer, and correct way to implement this API. Let me know your idea about this. Thanks, Quynh
On Wed, Jul 22, 2009 at 04:27:45PM +0900, Nguyen Anh Quynh wrote:
On Wed, Jul 22, 2009 at 4:19 PM, Nguyen Anh Quynh<aquynh@gmail.com> wrote: Acutally, to avoid all those ugly sanity checks, it is best to define
S/ugly/sane/
VIR_MEMORY_* as an enum type, then redefine virDomainMemoryCheck() as (note the last param is changed):
int virDomainMemoryPeek (virDomainPtr dom, unsigned long long start, size_t size, void *buffer, enum virDomainMemoryFlags flags);
That is ugly, it's also wrong, it break API and ABI compatibility, forget about it !
Let me know your idea about this.
If more C was implemented with defensive programming, and if people didn't broke API every time they think "it would be nicer" then it would be way easier to actually develop in C ! Please change your mindset that just doesn't work in the long term, sorry ... 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/
On Wed, Jul 22, 2009 at 4:44 PM, Daniel Veillard<veillard@redhat.com> wrote:
On Wed, Jul 22, 2009 at 04:27:45PM +0900, Nguyen Anh Quynh wrote:
On Wed, Jul 22, 2009 at 4:19 PM, Nguyen Anh Quynh<aquynh@gmail.com> wrote: Acutally, to avoid all those ugly sanity checks, it is best to define
S/ugly/sane/
VIR_MEMORY_* as an enum type, then redefine virDomainMemoryCheck() as (note the last param is changed):
int virDomainMemoryPeek (virDomainPtr dom, unsigned long long start, size_t size, void *buffer, enum virDomainMemoryFlags flags);
That is ugly, it's also wrong, it break API and ABI compatibility, forget about it !
Let me know your idea about this.
If more C was implemented with defensive programming, and if people didn't broke API every time they think "it would be nicer" then it would be way easier to actually develop in C ! Please change your mindset that just doesn't work in the long term, sorry ...
Please take this new patch. Thank you, Quynh $ diffstat pmemsave4.diff include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 14 ++++++++------ src/qemu_driver.c | 15 ++++++++++----- 3 files changed, 19 insertions(+), 11 deletions(-)
On Wed, Jul 22, 2009 at 04:56:53PM +0900, Nguyen Anh Quynh wrote:
On Wed, Jul 22, 2009 at 4:44 PM, Daniel Veillard<veillard@redhat.com> wrote:
On Wed, Jul 22, 2009 at 04:27:45PM +0900, Nguyen Anh Quynh wrote:
On Wed, Jul 22, 2009 at 4:19 PM, Nguyen Anh Quynh<aquynh@gmail.com> wrote: Acutally, to avoid all those ugly sanity checks, it is best to define
S/ugly/sane/
VIR_MEMORY_* as an enum type, then redefine virDomainMemoryCheck() as (note the last param is changed):
int virDomainMemoryPeek (virDomainPtr dom, unsigned long long start, size_t size, void *buffer, enum virDomainMemoryFlags flags);
That is ugly, it's also wrong, it break API and ABI compatibility, forget about it !
Let me know your idea about this.
If more C was implemented with defensive programming, and if people didn't broke API every time they think "it would be nicer" then it would be way easier to actually develop in C ! Please change your mindset that just doesn't work in the long term, sorry ...
Please take this new patch.
ACK, looks reasonable. 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 :|
On Wed, Jul 22, 2009 at 04:56:53PM +0900, Nguyen Anh Quynh wrote:
On Wed, Jul 22, 2009 at 4:44 PM, Daniel Veillard<veillard@redhat.com> wrote:
On Wed, Jul 22, 2009 at 04:27:45PM +0900, Nguyen Anh Quynh wrote:
On Wed, Jul 22, 2009 at 4:19 PM, Nguyen Anh Quynh<aquynh@gmail.com> wrote: Acutally, to avoid all those ugly sanity checks, it is best to define
S/ugly/sane/
VIR_MEMORY_* as an enum type, then redefine virDomainMemoryCheck() as (note the last param is changed):
int virDomainMemoryPeek (virDomainPtr dom, unsigned long long start, size_t size, void *buffer, enum virDomainMemoryFlags flags);
That is ugly, it's also wrong, it break API and ABI compatibility, forget about it !
Let me know your idea about this.
If more C was implemented with defensive programming, and if people didn't broke API every time they think "it would be nicer" then it would be way easier to actually develop in C ! Please change your mindset that just doesn't work in the long term, sorry ...
Please take this new patch.
Okay, now that's clean :-) Applied and commited, thank you ! 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/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Nguyen Anh Quynh