| Message ID | 20230606144551.24367-2-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Fix multifd cancel test | expand |
On Tue, Jun 06, 2023 at 11:45:49AM -0300, Fabiano Rosas wrote: > The code in threadinfo.c is only used for the QMP command > query-migrationthreads. Make it explicit that this is something > related to QMP. > > The current names are also too generic for a piece of code that > doesn't affect the migration directly in any way. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> Looks good here, but shall we reserve the qmp_* prefix to mostly qmp stuff only? Dropping "qmp_" in the new names would look better to me.. > --- > migration/migration.c | 4 ++-- > migration/multifd.c | 4 ++-- > migration/threadinfo.c | 4 ++-- > migration/threadinfo.h | 5 ++--- > 4 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index dc05c6f6ea..e731fc98a1 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2922,7 +2922,7 @@ static void *migration_thread(void *opaque) > MigThrError thr_error; > bool urgent = false; > > - thread = MigrationThreadAdd("live_migration", qemu_get_thread_id()); > + thread = qmp_migration_threads_add("live_migration", qemu_get_thread_id()); > > rcu_register_thread(); > > @@ -3000,7 +3000,7 @@ static void *migration_thread(void *opaque) > migration_iteration_finish(s); > object_unref(OBJECT(s)); > rcu_unregister_thread(); > - MigrationThreadDel(thread); > + qmp_migration_threads_remove(thread); > return NULL; > } > > diff --git a/migration/multifd.c b/migration/multifd.c > index 0bf5958a9c..5ec1ac5c64 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -651,7 +651,7 @@ static void *multifd_send_thread(void *opaque) > int ret = 0; > bool use_zero_copy_send = migrate_zero_copy_send(); > > - thread = MigrationThreadAdd(p->name, qemu_get_thread_id()); > + thread = qmp_migration_threads_add(p->name, qemu_get_thread_id()); > > trace_multifd_send_thread_start(p->id); > rcu_register_thread(); > @@ -767,7 +767,7 @@ out: > qemu_mutex_unlock(&p->mutex); > > rcu_unregister_thread(); > - MigrationThreadDel(thread); > + qmp_migration_threads_remove(thread); > trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages); > > return NULL; > diff --git a/migration/threadinfo.c b/migration/threadinfo.c > index 1de8b31855..c3e85c33e8 100644 > --- a/migration/threadinfo.c > +++ b/migration/threadinfo.c > @@ -14,7 +14,7 @@ > > static QLIST_HEAD(, MigrationThread) migration_threads; > > -MigrationThread *MigrationThreadAdd(const char *name, int thread_id) > +MigrationThread *qmp_migration_threads_add(const char *name, int thread_id) > { > MigrationThread *thread = g_new0(MigrationThread, 1); > thread->name = name; > @@ -25,7 +25,7 @@ MigrationThread *MigrationThreadAdd(const char *name, int thread_id) > return thread; > } > > -void MigrationThreadDel(MigrationThread *thread) > +void qmp_migration_threads_remove(MigrationThread *thread) > { > if (thread) { > QLIST_REMOVE(thread, node); > diff --git a/migration/threadinfo.h b/migration/threadinfo.h > index 4d69423c0a..61b990f5e3 100644 > --- a/migration/threadinfo.h > +++ b/migration/threadinfo.h > @@ -23,6 +23,5 @@ struct MigrationThread { > QLIST_ENTRY(MigrationThread) node; > }; > > -MigrationThread *MigrationThreadAdd(const char *name, int thread_id); > - > -void MigrationThreadDel(MigrationThread *info); > +MigrationThread *qmp_migration_threads_add(const char *name, int thread_id); > +void qmp_migration_threads_remove(MigrationThread *info); > -- > 2.35.3 >
Peter Xu <peterx@redhat.com> writes: > On Tue, Jun 06, 2023 at 11:45:49AM -0300, Fabiano Rosas wrote: >> The code in threadinfo.c is only used for the QMP command >> query-migrationthreads. Make it explicit that this is something >> related to QMP. >> >> The current names are also too generic for a piece of code that >> doesn't affect the migration directly in any way. >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> > > Looks good here, but shall we reserve the qmp_* prefix to mostly qmp stuff > only? Dropping "qmp_" in the new names would look better to me.. > Well, we're just putting the thread name and id on a list so that QMP can use them later. It is nothing "important" enough to have a generic name like migration_thread. Perhaps: thread_info_add thread_info_remove thread_info_init thread_info_cleanup Anyway, as long as we drop that camel case I'm ok with just removing the qmp =)
On Tue, Jun 06, 2023 at 04:34:31PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Tue, Jun 06, 2023 at 11:45:49AM -0300, Fabiano Rosas wrote: > >> The code in threadinfo.c is only used for the QMP command > >> query-migrationthreads. Make it explicit that this is something > >> related to QMP. > >> > >> The current names are also too generic for a piece of code that > >> doesn't affect the migration directly in any way. > >> > >> Signed-off-by: Fabiano Rosas <farosas@suse.de> > > > > Looks good here, but shall we reserve the qmp_* prefix to mostly qmp stuff > > only? Dropping "qmp_" in the new names would look better to me.. > > > > Well, we're just putting the thread name and id on a list so that QMP > can use them later. It is nothing "important" enough to have a generic > name like migration_thread. > > Perhaps: > > thread_info_add > thread_info_remove > thread_info_init > thread_info_cleanup > > Anyway, as long as we drop that camel case I'm ok with just removing the > qmp =) Thanks. To me OTOH it's good as long as "qmp_" dropped. :) I don't worry on using "migration_thread_" as prefix, that's exactly what the api does to me. Or, migration_thread_info_*(), migration_thr_mgr_*(), etc.
Fabiano Rosas <farosas@suse.de> wrote: > The code in threadinfo.c is only used for the QMP command > query-migrationthreads. Make it explicit that this is something > related to QMP. > > The current names are also too generic for a piece of code that > doesn't affect the migration directly in any way. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Don't like the caml case. And don't really care with/without the qmp_ preffix. You got it eitherwise.
On 6/6/23 16:45, Fabiano Rosas wrote: > The code in threadinfo.c is only used for the QMP command > query-migrationthreads. Make it explicit that this is something > related to QMP. > > The current names are also too generic for a piece of code that > doesn't affect the migration directly in any way. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > migration/migration.c | 4 ++-- > migration/multifd.c | 4 ++-- > migration/threadinfo.c | 4 ++-- > migration/threadinfo.h | 5 ++--- > 4 files changed, 8 insertions(+), 9 deletions(-) > -MigrationThread *MigrationThreadAdd(const char *name, int thread_id); > - > -void MigrationThreadDel(MigrationThread *info); > +MigrationThread *qmp_migration_threads_add(const char *name, int thread_id); > +void qmp_migration_threads_remove(MigrationThread *info); Dropping 'qmp_' prefix: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/migration/migration.c b/migration/migration.c index dc05c6f6ea..e731fc98a1 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2922,7 +2922,7 @@ static void *migration_thread(void *opaque) MigThrError thr_error; bool urgent = false; - thread = MigrationThreadAdd("live_migration", qemu_get_thread_id()); + thread = qmp_migration_threads_add("live_migration", qemu_get_thread_id()); rcu_register_thread(); @@ -3000,7 +3000,7 @@ static void *migration_thread(void *opaque) migration_iteration_finish(s); object_unref(OBJECT(s)); rcu_unregister_thread(); - MigrationThreadDel(thread); + qmp_migration_threads_remove(thread); return NULL; } diff --git a/migration/multifd.c b/migration/multifd.c index 0bf5958a9c..5ec1ac5c64 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -651,7 +651,7 @@ static void *multifd_send_thread(void *opaque) int ret = 0; bool use_zero_copy_send = migrate_zero_copy_send(); - thread = MigrationThreadAdd(p->name, qemu_get_thread_id()); + thread = qmp_migration_threads_add(p->name, qemu_get_thread_id()); trace_multifd_send_thread_start(p->id); rcu_register_thread(); @@ -767,7 +767,7 @@ out: qemu_mutex_unlock(&p->mutex); rcu_unregister_thread(); - MigrationThreadDel(thread); + qmp_migration_threads_remove(thread); trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages); return NULL; diff --git a/migration/threadinfo.c b/migration/threadinfo.c index 1de8b31855..c3e85c33e8 100644 --- a/migration/threadinfo.c +++ b/migration/threadinfo.c @@ -14,7 +14,7 @@ static QLIST_HEAD(, MigrationThread) migration_threads; -MigrationThread *MigrationThreadAdd(const char *name, int thread_id) +MigrationThread *qmp_migration_threads_add(const char *name, int thread_id) { MigrationThread *thread = g_new0(MigrationThread, 1); thread->name = name; @@ -25,7 +25,7 @@ MigrationThread *MigrationThreadAdd(const char *name, int thread_id) return thread; } -void MigrationThreadDel(MigrationThread *thread) +void qmp_migration_threads_remove(MigrationThread *thread) { if (thread) { QLIST_REMOVE(thread, node); diff --git a/migration/threadinfo.h b/migration/threadinfo.h index 4d69423c0a..61b990f5e3 100644 --- a/migration/threadinfo.h +++ b/migration/threadinfo.h @@ -23,6 +23,5 @@ struct MigrationThread { QLIST_ENTRY(MigrationThread) node; }; -MigrationThread *MigrationThreadAdd(const char *name, int thread_id); - -void MigrationThreadDel(MigrationThread *info); +MigrationThread *qmp_migration_threads_add(const char *name, int thread_id); +void qmp_migration_threads_remove(MigrationThread *info);
The code in threadinfo.c is only used for the QMP command query-migrationthreads. Make it explicit that this is something related to QMP. The current names are also too generic for a piece of code that doesn't affect the migration directly in any way. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/migration.c | 4 ++-- migration/multifd.c | 4 ++-- migration/threadinfo.c | 4 ++-- migration/threadinfo.h | 5 ++--- 4 files changed, 8 insertions(+), 9 deletions(-)