Re: [PATCH V1] migration: export fewer options

[Peter Xu <peterx@redhat.com>]

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

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

-- 
Peter Xu