Re: [PATCH v3 1/1] vhost-user-fs: add migration type property

[Anton Kuchin]

On 22/02/2023 14:20, Vladimir Sementsov-Ogievskiy wrote:
> On 17.02.23 20:00, Anton Kuchin wrote:
> > Migration of vhost-user-fs device requires transfer of FUSE internal 
> > state
> > from backend. There is no standard way to do it now so by default 
> > migration
> > must be blocked. But if this state can be externally transferred by
> > orchestrator give it an option to explicitly allow migration.
> >
> > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
> > ---
> >   hw/core/qdev-properties-system.c    | 10 +++++++++
> >   hw/virtio/vhost-user-fs.c           | 32 ++++++++++++++++++++++++++++-
> >   include/hw/qdev-properties-system.h |  1 +
> >   include/hw/virtio/vhost-user-fs.h   |  2 ++
> >   qapi/migration.json                 | 16 +++++++++++++++
> >   5 files changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/core/qdev-properties-system.c 
> > b/hw/core/qdev-properties-system.c
> > index d42493f630..d9b1aa2a5d 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -1143,3 +1143,13 @@ const PropertyInfo qdev_prop_uuid = {
> >       .set   = set_uuid,
> >       .set_default_value = set_default_uuid_auto,
> >   };
> > +
> > +const PropertyInfo qdev_prop_vhost_user_migration_type = {
> > +    .name = "VhostUserMigrationType",
> > +    .description = "none/external",
> > +    .enum_table = &VhostUserMigrationType_lookup,
> > +    .get = qdev_propinfo_get_enum,
> > +    .set = qdev_propinfo_set_enum,
> > +    .set_default_value = qdev_propinfo_set_default_value_enum,
> > +    .realized_set_allowed = true,
> > +};
> > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > index 83fc20e49e..7deb9df5ec 100644
> > --- a/hw/virtio/vhost-user-fs.c
> > +++ b/hw/virtio/vhost-user-fs.c
> > @@ -298,9 +298,35 @@ static struct vhost_dev 
> > *vuf_get_vhost(VirtIODevice *vdev)
> >       return &fs->vhost_dev;
> >   }
> >   +static int vhost_user_fs_pre_save(void *opaque)
> > +{
> > +    VHostUserFS *fs = opaque;
> > +    g_autofree char *path = object_get_canonical_path(OBJECT(fs));
> > +
> > +    switch (fs->migration_type) {
> > +    case VHOST_USER_MIGRATION_TYPE_NONE:
> > +        error_report("Migration is blocked by device %s", path);
> > +        break;
> > +    case VHOST_USER_MIGRATION_TYPE_EXTERNAL:
> > +        return 0;
> > +    default:
> > +        error_report("Migration type '%s' is not supported by device 
> > %s",
> > + VhostUserMigrationType_str(fs->migration_type), path);
> > +        break;
> > +    }
> > +
> > +    return -1;
> > +}
>
> Should we also add this as .pre_load, to force user select correct 
> migration_type on target too?

Why do we need it? Enum forces user to select at least one of the sane 
option
and I believe this is enough. As this property affects only save and 
don't know
what we can do at load.

> > +
> >   static const VMStateDescription vuf_vmstate = {
> >       .name = "vhost-user-fs",
> > -    .unmigratable = 1,
> > +    .minimum_version_id = 0,
> > +    .version_id = 0,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_VIRTIO_DEVICE,
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +   .pre_save = vhost_user_fs_pre_save,
> >   };
> >     static Property vuf_properties[] = {
> > @@ -309,6 +335,10 @@ static Property vuf_properties[] = {
> >       DEFINE_PROP_UINT16("num-request-queues", VHostUserFS,
> >                          conf.num_request_queues, 1),
> >       DEFINE_PROP_UINT16("queue-size", VHostUserFS, conf.queue_size, 
> > 128),
> > +    DEFINE_PROP_UNSIGNED("migration", VHostUserFS, migration_type,
> > +                         VHOST_USER_MIGRATION_TYPE_NONE,
> > +                         qdev_prop_vhost_user_migration_type,
> > +                         VhostUserMigrationType),
>
> 1. I see, other similar qdev_prop_* use DEFINE_PROP_SIGNED

I don't think this should be signed. Enum values are non-negative so 
compilers
(at least gcc and clang that I checked) evaluate underlying enum type to 
be unsigned int.
I don't know why other property types use signed, may be they have 
reasons or just this
is how they were initially implemented.

> 2. All of them except only qdev_prop_fdc_drive_type, define also a 
> convenient macro in include/hw/qdev-properties-system.h

This makes sense if property is used in more than one place, in this 
case I don't see any
benefit from writing more code to handle this specific case. Maybe if 
property finds its
usage in other devices this can be done.

> should we follow these patterns?