* Re: [PATCH] hostmem: Don't report pmem attribute if unsupported
2021-01-19 12:35 [PATCH] hostmem: Don't report pmem attribute if unsupported Michal Privoznik
@ 2021-01-19 16:55 ` Daniel Henrique Barboza
2021-01-20 10:07 ` Igor Mammedov
1 sibling, 0 replies; 3+ messages in thread
From: Daniel Henrique Barboza @ 2021-01-19 16:55 UTC (permalink / raw)
To: Michal Privoznik, qemu-devel
On 1/19/21 9:35 AM, Michal Privoznik wrote:
> When management applications (like Libvirt) want to check whether
> memory-backend-file.pmem is supported they can list object
> properties using 'qom-list-properties'. However, 'pmem' is
> declared always (and thus reported always) and only at runtime
> QEMU errors out if it was built without libpmem (and thus can not
> guarantee write persistence). This is suboptimal since we have
> ability to declare attributes at compile time.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1915216
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
Testing it on a host that doesn't have libpmem support:
- without this patch:
{"execute": "qom-list-properties", "arguments": {"typename": "memory-backend-file"}}
{"return": [
(...)
{"name": "pmem", "type": "bool"},
(...)
]}
- with this patch there is no 'pmem' support reported:
{"execute": "qom-list-properties", "arguments": {"typename": "memory-backend-file"}}
{"return": [
{"name": "type", "type": "string"},
{"name": "policy", "description": "Set the NUMA policy", "type": "HostMemPolicy"},
{"name": "prealloc-threads", "description": "Number of CPU threads to use for prealloc", "type": "int"},
{"name": "dump", "description": "Set to 'off' to exclude from core dump", "type": "bool"},
{"name": "share", "description": "Mark the memory as private to QEMU or shared", "type": "bool"},
{"name": "prealloc", "description": "Preallocate memory", "type": "bool"},
{"name": "size", "description": "Size of the memory region (ex: 500M)", "type": "int"},
{"name": "x-use-canonical-path-for-ramblock-id", "type": "bool"},
{"name": "host-nodes", "description": "Binds memory to the list of NUMA host nodes", "type": "int"},
{"name": "merge", "description": "Mark memory as mergeable", "type": "bool"},
{"name": "align", "type": "int"},
{"name": "mem-path", "type": "string"},
{"name": "discard-data", "type": "bool"}
]}
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> backends/hostmem-file.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 40e1e5b3e3..7e30eb5985 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -123,6 +123,7 @@ static void file_memory_backend_set_align(Object *o, Visitor *v,
> fb->align = val;
> }
>
> +#ifdef CONFIG_LIBPMEM
> static bool file_memory_backend_get_pmem(Object *o, Error **errp)
> {
> return MEMORY_BACKEND_FILE(o)->is_pmem;
> @@ -139,17 +140,9 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> return;
> }
>
> -#ifndef CONFIG_LIBPMEM
> - if (value) {
> - error_setg(errp, "Lack of libpmem support while setting the 'pmem=on'"
> - " of %s. We can't ensure data persistence.",
> - object_get_typename(o));
> - return;
> - }
> -#endif
> -
> fb->is_pmem = value;
> }
> +#endif /* CONFIG_LIBPMEM */
>
> static void file_backend_unparent(Object *obj)
> {
> @@ -180,8 +173,10 @@ file_backend_class_init(ObjectClass *oc, void *data)
> file_memory_backend_get_align,
> file_memory_backend_set_align,
> NULL, NULL);
> +#ifdef CONFIG_LIBPMEM
> object_class_property_add_bool(oc, "pmem",
> file_memory_backend_get_pmem, file_memory_backend_set_pmem);
> +#endif
> }
>
> static void file_backend_instance_finalize(Object *o)
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] hostmem: Don't report pmem attribute if unsupported
2021-01-19 12:35 [PATCH] hostmem: Don't report pmem attribute if unsupported Michal Privoznik
2021-01-19 16:55 ` Daniel Henrique Barboza
@ 2021-01-20 10:07 ` Igor Mammedov
1 sibling, 0 replies; 3+ messages in thread
From: Igor Mammedov @ 2021-01-20 10:07 UTC (permalink / raw)
To: Michal Privoznik; +Cc: qemu-devel
On Tue, 19 Jan 2021 13:35:35 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:
> When management applications (like Libvirt) want to check whether
> memory-backend-file.pmem is supported they can list object
> properties using 'qom-list-properties'. However, 'pmem' is
> declared always (and thus reported always) and only at runtime
> QEMU errors out if it was built without libpmem (and thus can not
> guarantee write persistence). This is suboptimal since we have
> ability to declare attributes at compile time.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1915216
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> backends/hostmem-file.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 40e1e5b3e3..7e30eb5985 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -123,6 +123,7 @@ static void file_memory_backend_set_align(Object *o, Visitor *v,
> fb->align = val;
> }
>
> +#ifdef CONFIG_LIBPMEM
> static bool file_memory_backend_get_pmem(Object *o, Error **errp)
> {
> return MEMORY_BACKEND_FILE(o)->is_pmem;
> @@ -139,17 +140,9 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> return;
> }
>
> -#ifndef CONFIG_LIBPMEM
> - if (value) {
> - error_setg(errp, "Lack of libpmem support while setting the 'pmem=on'"
> - " of %s. We can't ensure data persistence.",
> - object_get_typename(o));
> - return;
> - }
> -#endif
> -
> fb->is_pmem = value;
> }
> +#endif /* CONFIG_LIBPMEM */
>
> static void file_backend_unparent(Object *obj)
> {
> @@ -180,8 +173,10 @@ file_backend_class_init(ObjectClass *oc, void *data)
> file_memory_backend_get_align,
> file_memory_backend_set_align,
> NULL, NULL);
> +#ifdef CONFIG_LIBPMEM
> object_class_property_add_bool(oc, "pmem",
> file_memory_backend_get_pmem, file_memory_backend_set_pmem);
> +#endif
> }
>
> static void file_backend_instance_finalize(Object *o)
^ permalink raw reply [flat|nested] 3+ messages in thread