| Message ID | 1708708404-197951-1-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V1] migration: export fewer options | expand |
On Fri, Feb 23, 2024 at 09:13:24AM -0800, Steve Sistare wrote: > 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> Sounds reasonable, queued, thanks. > --- > 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. What's the difference v.s. current migration/misc.h?
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. [...]
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? 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. - Steve
On 2/25/2024 9:40 PM, Peter Xu wrote: > On Fri, Feb 23, 2024 at 09:13:24AM -0800, Steve Sistare wrote: >> 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> > > Sounds reasonable, queued, thanks. > >> --- >> 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. > > What's the difference v.s. current migration/misc.h? This file would be sufficient for most clients: diff --git a/include/migration/client.h b/include/migration/client.h new file mode 100644 index 0000000..a55e504 --- /dev/null +++ b/include/migration/client.h @@ -0,0 +1,6 @@ +#ifndef MIGRATION_CLIENT_H +#define MIGRATION_CLIENT_H +#include "migration/misc.h" +#include "migration/blocker.h" +#include "migration/client-options.h" +#endif Or, we could rename misc.h -> client.h and include blocker.h and client-options.h in it. I just like the idea that most clients could include a single, obviously named file to use the most-common exported API. "misc.h" is not obvious, and not complete. - Steve
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..]
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..] >
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 86d8c9e..f1f6878 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -18,7 +18,6 @@ #include "sysemu/runstate.h" #include "hw/vfio/vfio-common.h" #include "migration/migration.h" -#include "migration/options.h" #include "migration/savevm.h" #include "migration/vmstate.h" #include "migration/qemu-file.h" diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index d004cf2..74d419f 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -32,7 +32,6 @@ #include "qemu/error-report.h" #include "migration/misc.h" #include "migration/migration.h" -#include "migration/options.h" #include "hw/virtio/virtio-bus.h" #include "hw/virtio/virtio-access.h" diff --git a/include/migration/misc.h b/include/migration/misc.h index 916b65f..627726c 100644 --- a/include/migration/misc.h +++ b/include/migration/misc.h @@ -17,6 +17,7 @@ #include "qemu/notify.h" #include "qapi/qapi-types-migration.h" #include "qapi/qapi-types-net.h" +#include "migration/options-pub.h" /* migration/ram.c */ diff --git a/include/migration/options-pub.h b/include/migration/options-pub.h new file mode 100644 index 0000000..54bd678 --- /dev/null +++ b/include/migration/options-pub.h @@ -0,0 +1,24 @@ +/* + * QEMU public migration capabilities + * + * Copyright (c) 2012-2023 Red Hat Inc + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#ifndef QEMU_MIGRATION_OPTIONS_PUB_H +#define QEMU_MIGRATION_OPTIONS_PUB_H + +/* capabilities */ + +bool migrate_background_snapshot(void); +bool migrate_dirty_limit(void); +bool migrate_postcopy_ram(void); +bool migrate_switchover_ack(void); + +/* parameters */ + +MigMode migrate_mode(void); + +#endif diff --git a/migration/options.h b/migration/options.h index 246c160..84c08e1 100644 --- a/migration/options.h +++ b/migration/options.h @@ -16,6 +16,7 @@ #include "hw/qdev-properties.h" #include "hw/qdev-properties-system.h" +#include "migration/options-pub.h" /* migration properties */ @@ -24,12 +25,10 @@ extern Property migration_properties[]; /* capabilities */ bool migrate_auto_converge(void); -bool migrate_background_snapshot(void); bool migrate_block(void); bool migrate_colo(void); bool migrate_compress(void); bool migrate_dirty_bitmaps(void); -bool migrate_dirty_limit(void); bool migrate_events(void); bool migrate_ignore_shared(void); bool migrate_late_block_activate(void); @@ -37,11 +36,9 @@ bool migrate_multifd(void); bool migrate_pause_before_switchover(void); bool migrate_postcopy_blocktime(void); bool migrate_postcopy_preempt(void); -bool migrate_postcopy_ram(void); bool migrate_rdma_pin_all(void); bool migrate_release_ram(void); bool migrate_return_path(void); -bool migrate_switchover_ack(void); bool migrate_validate_uuid(void); bool migrate_xbzrle(void); bool migrate_zero_blocks(void); @@ -83,7 +80,6 @@ uint8_t migrate_max_cpu_throttle(void); uint64_t migrate_max_bandwidth(void); uint64_t migrate_avail_switchover_bandwidth(void); uint64_t migrate_max_postcopy_bandwidth(void); -MigMode migrate_mode(void); int migrate_multifd_channels(void); MultiFDCompression migrate_multifd_compression(void); int migrate_multifd_zlib_level(void); diff --git a/system/dirtylimit.c b/system/dirtylimit.c index 495c7a7..696eaab 100644 --- a/system/dirtylimit.c +++ b/system/dirtylimit.c @@ -26,7 +26,6 @@ #include "trace.h" #include "migration/misc.h" #include "migration/migration.h" -#include "migration/options.h" /* * Dirtylimit stop working if dirty page rate error
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 +----- system/dirtylimit.c | 1 - 6 files changed, 26 insertions(+), 8 deletions(-) create mode 100644 include/migration/options-pub.h