Re: [PATCH QEMU HCI-8.1 12/21] migration: Move CPR HUP watch to cpr-transfer.c

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: cpr_transfer_add_hup_watch() takes untyped opaque but needs MigrationAddress
--

commit c7d2819ddcd0f6868aa501a95d87ca2adba9d5d9
Author: Fabiano Rosas <farosas@suse.de>

migration: Move CPR HUP watch to cpr-transfer.c

This commit moves the CPR HUP watch helpers out of migration.c into
cpr-transfer.c and renames them cpr_transfer_add_hup_watch() and
cpr_transfer_source_destroy(). Callers in migration_cleanup(),
migration_cancel(), qmp_migrate_finish_cb() and qmp_migrate() are
updated to use the new names.

Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-24-farosas@suse.de

> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
> --- a/include/migration/cpr.h
> +++ b/include/migration/cpr.h

[ ... ]

> @@ -76,6 +77,10 @@ int cpr_get_fd_param(const char *name, const char *fdname, int index, bool cpr,
>  QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp);
>  QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp);
>  
> +void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
> +                                void *opaque);
> +void cpr_transfer_source_destroy(MigrationState *s);

[Severity: Low]
This isn't a bug today, but the prototype of cpr_transfer_add_hup_watch()
takes a generic "void *opaque", while the implementation in
migration/cpr-transfer.c always does
QAPI_CLONE(MigrationAddress, opaque) and registers
qapi_free_MigrationAddress as the destroy notifier.

Could the parameter be declared as "MigrationAddress *addr" instead? A
future caller passing any other pointer type would compile cleanly, and
the function would then clone and free the wrong object.

The only caller is qmp_migrate(), which passes addr, a MigrationAddress *,
so behaviour is preserved.

> diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
> --- a/migration/cpr-transfer.c
> +++ b/migration/cpr-transfer.c

[ ... ]

> +void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
> +                                void *opaque)
> +{
> +    s->hup_source = qio_channel_create_watch(cpr_state_ioc(), G_IO_HUP);
> +    g_source_set_callback(s->hup_source,
> +                          (GSourceFunc)func,
> +                          QAPI_CLONE(MigrationAddress, opaque),
> +                          (GDestroyNotify)qapi_free_MigrationAddress);
> +    g_source_attach(s->hup_source, NULL);
> +}

The same untyped "void *opaque" appears here, and the contract that it
must be a MigrationAddress is only visible inside the function body.

The old migrate_hup_add() also took "void *opaque", and the series
through 3953515702de does not change this, so a typed parameter would
have to be done here.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=12