[RESEND] hostmem: Don't report pmem attribute if unsupported

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: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---

This is just a resend of a patch I've sent earlier with Reviewed-by and
Tested-by added:

https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg04558.html

 backends/hostmem-file.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

On 1/26/21 8:48 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>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> 
> This is just a resend of a patch I've sent earlier with Reviewed-by and
> Tested-by added:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg04558.html
> 
>   backends/hostmem-file.c | 13 ++++---------
>   1 file changed, 4 insertions(+), 9 deletions(-)

Polite ping, please.

Michal

On Tue, Jan 26, 2021 at 08:48:25AM +0100, 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>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

I'm not a fan of making availability of properties conditional
(even if at compile time), but if this helps libvirt I guess it
makes sense.

CCing John, who has been thinking a lot about these questions.

I'm queueing this on machine-next.  Thanks, and sorry for the delay!

> ---
> 
> This is just a resend of a patch I've sent earlier with Reviewed-by and
> Tested-by added:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg04558.html
> 
>  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)
> -- 
> 2.26.2
>

On 2/16/21 5:23 PM, Eduardo Habkost wrote:
> On Tue, Jan 26, 2021 at 08:48:25AM +0100, 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>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> I'm not a fan of making availability of properties conditional
> (even if at compile time), but if this helps libvirt I guess it
> makes sense.
> 

Compile time might be OK, but if we want to describe everything via QAPI 
eventually, we just need to be able to describe that compile-time 
requisite appropriately.

Conditional at run-time is I think the thing that absolutely has to go 
wherever it surfaces.

> CCing John, who has been thinking a lot about these questions.
> 

Thanks for the heads up. Good reminder that libvirt uses the existence 
of properties as a bellwether for feature support. I don't think I like 
that idea, but I like breaking libvirt even less.

--js

> I'm queueing this on machine-next.  Thanks, and sorry for the delay!
> 
>> ---
>>
>> This is just a resend of a patch I've sent earlier with Reviewed-by and
>> Tested-by added:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg04558.html
>>
>>   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)
>> -- 
>> 2.26.2
>>
>

On 2/17/21 12:07 AM, John Snow wrote:
> On 2/16/21 5:23 PM, Eduardo Habkost wrote:
>> On Tue, Jan 26, 2021 at 08:48:25AM +0100, 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>
>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>
>> I'm not a fan of making availability of properties conditional
>> (even if at compile time), but if this helps libvirt I guess it
>> makes sense.
>>
> 
> Compile time might be OK, but if we want to describe everything via QAPI 
> eventually, we just need to be able to describe that compile-time 
> requisite appropriately.
> 
> Conditional at run-time is I think the thing that absolutely has to go 
> wherever it surfaces.

I'm open for discussion. How do you think libvirt (or any other mgmt 
tool/user) should inspect whether given feature is available?
What libvirt currently does it issues 'qom-list-properties' with 
'typename=memory-backend-file' and inspects whether pmem attribute is 
available. Is 'qom-list' preferred?


> 
>> CCing John, who has been thinking a lot about these questions.
>>
> 
> Thanks for the heads up. Good reminder that libvirt uses the existence 
> of properties as a bellwether for feature support. I don't think I like 
> that idea, but I like breaking libvirt even less.

That was at hand solution. If libvirt's not doing it right, I'm happy to 
make things better.

Michal

On 2/17/21 2:31 AM, Michal Privoznik wrote:
> On 2/17/21 12:07 AM, John Snow wrote:
>> On 2/16/21 5:23 PM, Eduardo Habkost wrote:
>>> On Tue, Jan 26, 2021 at 08:48:25AM +0100, 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>
>>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>
>>> I'm not a fan of making availability of properties conditional
>>> (even if at compile time), but if this helps libvirt I guess it
>>> makes sense.
>>>
>>
>> Compile time might be OK, but if we want to describe everything via 
>> QAPI eventually, we just need to be able to describe that compile-time 
>> requisite appropriately.
>>
>> Conditional at run-time is I think the thing that absolutely has to go 
>> wherever it surfaces.
> 
> I'm open for discussion. How do you think libvirt (or any other mgmt 
> tool/user) should inspect whether given feature is available?
> What libvirt currently does it issues 'qom-list-properties' with 
> 'typename=memory-backend-file' and inspects whether pmem attribute is 
> available. Is 'qom-list' preferred?
> 
> 
>>
>>> CCing John, who has been thinking a lot about these questions.
>>>
>>
>> Thanks for the heads up. Good reminder that libvirt uses the existence 
>> of properties as a bellwether for feature support. I don't think I 
>> like that idea, but I like breaking libvirt even less.
> 
> That was at hand solution. If libvirt's not doing it right, I'm happy to 
> make things better.
> 
> Michal

No, libvirt is doing it exactly correct. QAPI/QMP was designed exactly 
in this way with exactly this use-case in mind.

(So far as I understand it.)

My concerns that may have guided some patches by Eduardo that might have 
caused problems for you relate to my ability to publish an SDK for 
generic builds of QEMU, where if-conditionals that actually compile 
fields out of certain data structures can be difficult to deal with at 
static analysis time.

Until we connect to the server, we don't know if type FooStruct has 
field XYZ or not. Generally the way you handle this is by always having 
that field in the SDK and erroring out at runtime if for some reason it 
is not supportable.

In the long term, we want to (I think) bridge the data gap between QOM 
and QAPI and provide a unified set of types that we can use to construct 
a "QEMU Config File" that can be validated statically against, say, 
"qemu 6.0."

In this scenario, I have some nebulous but not necessarily meticulously 
reasoned out concerns about compile-time conditional fields. In this 
scenario, using the presence or absence of a field in a data type 
becomes a poor way to do feature detection.

QMP offers the "features" flag for certain commands where we use the 
presence or absence of that flag as the introspection data in order to 
determine behavior. Going forward, I suspect I will push for 
representing formerly-compile-time flags as runtime introspection 
feature flags instead.

...But that's stuff that isn't here now, so just keep doing what you've 
been doing, and I will take careful notice not to break that kind of 
introspection without a well-advertised alternative.

Especially right now, QOM stuff isn't in QAPI, so we don't have those 
kind of feature flags at all, so I think there really isn't another way 
at all, short of adding more capabilities and complexity to the existing 
introspection stuff, which I don't think we'd do.

--js