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
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
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
--
1.8.3.1Steve 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 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
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..] >
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? -- Peter Xu
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
© 2016 - 2024 Red Hat, Inc.