Re: [PATCH V1] migration: export fewer options

[Steven Sistare]

On 2/27/2024 3:08 AM, Peter Xu wrote:
> On Mon, Feb 26, 2024 at 09:41:15AM -0500, Steven Sistare wrote:
>> On 2/26/2024 2:40 AM, Markus Armbruster wrote:
>>> Steve Sistare <steven.sistare@oracle.com> writes:
>>>
>>>> A small number of migration options are accessed by migration clients,
>>>> but to see them clients must include all of options.h, which is mostly
>>>> for migration core code.  migrate_mode() in particular will be needed by
>>>> multiple clients.
>>>>
>>>> Refactor the option declarations so clients can see the necessary few via
>>>> misc.h, which already exports a portion of the client API.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>> I suggest that eventually we should define a single file migration/client.h
>>>> which exports everything needed by the simpler clients: blockers, 
>>>> notifiers,
>>>> options, cpr, and state accessors.
>>>> ---
>>>> ---
>>>>  hw/vfio/migration.c             |  1 -
>>>>  hw/virtio/virtio-balloon.c      |  1 -
>>>>  include/migration/misc.h        |  1 +
>>>>  include/migration/options-pub.h | 24 ++++++++++++++++++++++++
>>>>  migration/options.h             |  6 +-----
>>>
>>> Unusual naming.  We have zero headers named -pub.h or -public.h, and
>>> dozens named like -int.h or -internal.h.  Please stick to the existing
>>> convention.
>>
>> In the spirit of minimizing changes, I went that route to avoid renaming the 
>> existing migration/options.h and its references:
>>
>> 0 migration/block-dirty-bit 82 #include "options.h"
>> 1 migration/block.c         32 #include "options.h"
>> 2 migration/colo.c          37 #include "options.h"
>> 3 migration/migration-hmp-c 35 #include "options.h"
>> 4 migration/migration.c     68 #include "options.h"
>> 5 migration/multifd-zlib.c  21 #include "options.h"
>> 6 migration/multifd-zstd.c  21 #include "options.h"
>> 7 migration/multifd.c       29 #include "options.h"
>> 8 migration/options.c       30 #include "options.h"
>> 9 migration/postcopy-ram.c  40 #include "options.h"
>> a migration/qemu-file.c     33 #include "options.h"
>> b migration/ram-compress.c  37 #include "options.h"
>> c migration/ram.c           63 #include "options.h"
>> d migration/rdma.c          40 #include "options.h"
>> e migration/savevm.c        71 #include "options.h"
>> f migration/socket.c        30 #include "options.h"
>> g migration/tls.c           25 #include "options.h"
>>
>> But I take your point.
>>
>> Peter, which do you prefer?
> 
> From statistics, "-internal.h" wins "-int.h":
> 
> $ git grep "\-internal.h" | wc -l
> 135
> $ git grep "\-int.h" | wc -l
> 3

Yes, I did the same search to choose the name for option A below.
But in option B, I keep the existing name for the private file,
and choose a new name for the public file.  There is no suffix
in use in qemu to denote a public file; we just use names indicating
the functionality, so I chose client-options.h.

Let's use client-options.h and move on.  I am preparing another cleanup
series that I think you will like.

- Steve

>>   A. rename: migration/options.h -> migration/options-internal.h 
>>      rename: include/migration/options-pub.h -> include/migration/options.h
>>
>>   B. rename: include/migration/options.h -> 
>> include/migration/client-options.h
>>
>> I prefer B. If you prefer B, but want a different file name, please choose 
>> the
>> final name.
> 
> Personally I don't have a strong opinion on the name.  I'll see whether
> Markus has any comment.
> 
> [and of course, I removed this patch from -staging queue to keep the
>  discussion going..]
>