Re: [PATCH 10/22] rust: split "migration" crate

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4013 bytes --]

Hi

On Fri, Aug 29, 2025 at 12:37 PM Zhao Liu <zhao1.liu@intel.com> wrote:

> Hi Marc-Andre,
>
> On Wed, Aug 27, 2025 at 02:41:32PM +0400, marcandre.lureau@redhat.com
> wrote:
> > Date: Wed, 27 Aug 2025 14:41:32 +0400
> > From: marcandre.lureau@redhat.com
> > Subject: [PATCH 10/22] rust: split "migration" crate
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
>
> ...
>
> > @@ -0,0 +1,51 @@
> > +/*
> > + * QEMU System Emulator
> > + *
> > + * Copyright (c) 2024 Linaro Ltd.
> > + *
> > + * Authors: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> obtaining a copy
> > + * of this software and associated documentation files (the
> "Software"), to deal
> > + * in the Software without restriction, including without limitation
> the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be
> included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS IN
> > + * THE SOFTWARE.
> > + */
>
> Could we use /* SPDX-License-Identifier: GPL-2.0-or-later */ directly ?
>
> [snip]
>

I guess we could. Probably for a future cleanup though.


>
> > +// using extension traits would be nicer, unfortunately it doesn't
> allow const
> > +// fn yet
> > +pub struct VMStateFieldHelper(pub VMStateField);
> >
> >  // Add a couple builder-style methods to VMStateField, allowing
> >  // easy derivation of VMStateField constants from other types.
>
> A question: Sorry I didn't get your point about why we need
> VMStateFieldHelper?
>
> For its use case:
>
> > -        vmstate_struct!(FooB, arr_a[0 .. num_a], &VMSTATE_FOOA,
> FooA).with_version_id(1),
> > -        vmstate_struct!(FooB, arr_a_mul[0 .. num_a_mul * 32],
> &VMSTATE_FOOA, FooA).with_version_id(2),
> > +        VMStateFieldHelper(vmstate_struct!(FooB, arr_a[0 .. num_a],
> &VMSTATE_FOOA, FooA)).with_version_id(1).0,
> > +        VMStateFieldHelper(vmstate_struct!(FooB, arr_a_mul[0 ..
> num_a_mul * 32], &VMSTATE_FOOA, FooA)).with_version_id(2).0,
>
> It seems VMStateFieldHelper add another wrapper around vmstate_struct
> (and vmstate_of).
>
> The builder pattern is good, but I'm afraid this builder makes the use of
> vmstate_struct! more complex.
>
> > -impl VMStateField {
> > +impl VMStateFieldHelper {
> >      #[must_use]
> >      pub const fn with_version_id(mut self, version_id: i32) -> Self {
> >          assert!(version_id >= 0);
> > -        self.version_id = version_id;
> > +        self.0.version_id = version_id;
> >          self
> >      }
>
> If we could have a build() method then user doesn't need to write ".0"
> at the end.
>
> >  }
>
> And there's another VMStateDescriptionBuilder:
>
>
> https://lore.kernel.org/qemu-devel/20250505100854.73936-4-pbonzini@redhat.com/#t
>
> I think Paolo has the plan to merge it with v1.83 support. So if this
> VMStateFieldHelper is necessary, it's better seperate this into another
> patch and base it over VMStateDescriptionBuilder if possible.
>
>
Paolo rebased it and dropped the need for the VMStateFieldHelper.

[-- Attachment #2: Type: text/html, Size: 5426 bytes --]