Re: [PATCH] qapi: clarify that the default is backend dependent

[Markus Armbruster <armbru@redhat.com>]

Stefano Garzarella <sgarzare@redhat.com> writes:

> On Mon, Jun 03, 2024 at 11:34:10AM GMT, Markus Armbruster wrote:
>>Stefano Garzarella <sgarzare@redhat.com> writes:
>>
>>> The default value of the @share option of the @MemoryBackendProperties
>>> eally depends on the backend type, so let's document it explicitly and
>>> add the default value where it was missing.
>>>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>> ---
>>> I followed how we document @share in memfd and epc, but I don't like it
>>> very much, I just can't think of a better way, so if you have a suggestion
>>> I can change them in all of them.
>>>
>>> Thanks,
>>> Stefano
>>> ---
>>>  qapi/qom.json | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index 38dde6d785..8463bd32a2 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -600,7 +600,7 @@
>>   ##
>>   # @MemoryBackendProperties:
>>   #
>>   # Properties for objects of classes derived from memory-backend.
>>   #
>>
>>[...]
>>
>>>  #     preallocation threads (default: none) (since 7.2)
>>>  #
>>>  # @share: if false, the memory is private to QEMU; if true, it is
>>> -#     shared (default: false)
>>> +#     shared (default depends on the backend type)
>>
>>Note for later: the backends are the branches of ObjectOptions that use
>>MemoryBackendProperties as branch type or as base of their branch type.
>>These are
>>
>>    memory-backend-epc (uses MemoryBackendEpcProperties)
>>    memory-backend-file (uses MemoryBackendFileProperties)
>>    memory-backend-memfd (uses MemoryBackendMemfdProperties)
>>    memory-backend-ram (uses MemoryBackendProperties)
>>
>>>  #
>>>  # @reserve: if true, reserve swap space (or huge pages) if applicable
>>>  #     (default: true) (since 6.1)
>>> @@ -639,6 +639,8 @@
>>>  #
>>>  # Properties for memory-backend-file objects.
>>>  #
>>> +# The @share boolean option is false by default with file.
>>> +#
>>>  # @align: the base address alignment when QEMU mmap(2)s @mem-path.
>>>  #     Some backend stores specified by @mem-path require an alignment
>>>  #     different than the default one used by QEMU, e.g. the device DAX
>>
>>As stated in the commit message, this matches existing documentation in
>>memory-backend-epc
>>
>>   # The @share boolean option is true by default with epc
>>
>>and memory-backend-memfd
>>
>>   # The @share boolean option is true by default with memfd.
>>
>>I think "with FOO" could be clearer.  Perhaps something like "with
>>backend 'memory-backend-FOO'.
>
> Ack, I'll do.
>
>>
>>However, even with your patch, we're still missing memory-backend-ram.
>>I can see two solutions:
>>
>>1. Create MemoryBackendRamProperties just to have a place for
>>documenting @share's default.
>>
>>2. Document @share's default right where it's defined, roughly like
>>this:
>>
>>   # @share: if false, the memory is private to QEMU; if true, it is
>>   #     shared (default false for backends memory-backend-file and
>>   #     memory-backend-ram, true for backends memory-backend-epc and
>>   #     memory-backend-memfd)
>>
>>CON: we need to remember to update this whenever we add another backend.
>>
>>PRO: generated documentation is better, in my opinion.
>>
>>Thoughts?
>>
>
> Maybe option 2 is slightly better and it's also clearer how to document the default for other backends.
>
> When I added a new backend, it was not clear to me how to define the default for an inherited parameter.
>
> I would go with 2 if you agree.

I actually like 2 better :)