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

[John Snow <jsnow@redhat.com>]

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
>>
>