Re: [PATCH v11 00/10] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration

[Het Gala]

On 10/4/2023 9:02 PM, Fabiano Rosas wrote:

> Fabiano Rosas <farosas@suse.de> writes:
>
> > Het Gala <het.gala@nutanix.com> writes:
> >
> > > On 04/10/23 7:03 pm, Fabiano Rosas wrote:
> > > > Het Gala <het.gala@nutanix.com> writes:
> > > >
> > > > > This is v11 patchset of modified 'migrate' and 'migrate-incoming' QAPI design
> > > > > for upstream review.
> > > > >
> > > > > Update: Daniel has reviewed all patches and is okay with them. Markus has also
> > > > >          given Acked-by tag for patches related to QAPI syntax change.
> > > > > Fabiano, Juan and other migration maintainers, let me know if there are still
> > > > > improvements to be made in this patchset series.
> > > > >
> > > > > Link to previous upstream community patchset links:
> > > > > v1: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2022-2D12_msg04339.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=jsRvKRy1JOiy05KX1CtLqWN1su5XNmKPKuJTSx5sZpU&e=
> > > > > v2: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02106.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=mzt3n5PD1QclHfpZEh-VMoLkkwT8xqjPYN-1r7MOly0&e=
> > > > > v3: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02473.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=fa9W71JU6-3xZrjLH7AmElgqwJGUkPeQv3P7n6EXxOM&e=
> > > > > v4: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg03064.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=Xr1y3EvBzEtWT9O1fVNapCb3WnD-aWR8UeXv6J6gZQM&e=
> > > > > v5: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg04845.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=OtK10W2Z0DobrktRfTCMYPxbcMaaZ6f6qoA65D4RG_A&e=
> > > > > v6: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D06_msg01251.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=XH-4qFQgdkAKmRsa9DuqaZgJMvGUi1p4-s05AsAEYRo&e=
> > > > > v7: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02027.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=RwvfliI4wLm7S0TKl5RMku-gSSE-5fZPYH0MkzJdoPw&e=
> > > > > v8: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02770.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=BZsKBJGVPDWXwGgb2-fAnS9pWzTYuLzI92TmuWBcB3k&e=
> > > > > v9: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg04216.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=YcWFU9I2u-R6QbVjweZ3lFvJlllm-i9o5_jtLBxC_oc&e=
> > > > > v10: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg05022.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=JQt63Ikbz21vmsLmSensQu8zknGuS9bls-IFpndor78&e=
> > > > >
> > > > > v10 -> v11 changelog:
> > > > > -------------------
> > > > > - Resolved make check errors as its been almost two months since v10
> > > > >    version of this patchset series went out. Till date migration workflow
> > > > >    might have changed which caused make check errors.
> > > > >             
> > > > Sorry, there must be a misunderstanding here. This series still has
> > > > problems. Just look at patch 6 that adds the "channel-type" parameter and
> > > > patch 10 that uses "channeltype" in the test (without hyphen). This
> > > > cannot work.
> > > >           
> > > Ack. I will change that.
> > > > There's also several instances of g_autoptr being used incorrectly. I
> > > > could comment on every patch individually, but this series cannot have
> > > > passed make check.
> > > >           
> > > Are we allowed to run the make checks ? I am not aware from where these 
> > > failures are arising. It would be helpful if you could point out to me 
> > > where g_autoptr is incorrectly used ?
> > >         
> > I mean just the project's make check command:
> >
> > cd build/
> > ../configure
> > make -j$(nproc)
> > make -j$(nproc) check
> >       
>     
    Yes, I got it now. Thanks

> > > > Please resend this with the issues fixed and drop the Reviewed-bys from
> > > > the affected patches.
> > > >           
> > > How to verify which are the affected patches here ?
> > >         
> > I'll comment in each patch individually.
> >       
> Done.
>
> We had some double-frees when using g_autoptr in structures that are
> nested into another. The qapi code already descends and frees the
> children.
>
> There were also issues with allocating memory and later overwriting the
> pointers.
>
> This might still not put us in the most correct situation regarding
> memory, but I think it will at least get make check passing. Feel free
> to investigate the errors with make check and propose alternative
> solutions. It has been a while since I looked at this series, I might
> have missed something further.
>     
    Yes, for now I have tried to address all the comments made in the
    individual patches and tried to fix issue of pointer overwriting
    wherever I could spot, also double frees in the hmp code workflow.
    By doing this, I have passed all the make checks, but if there are
    places which needs to be re-looked for the above issues you
    mentioned, please let me know.

> > We'll also have to add compatibility with the new file: URI that's
> > included in the latest migration pull request. I'll add comments on
> > where I think we'll need to add code to support that feature.
> >       
> I'll actually defer here until you post your series with the
> fixes. It'll probably be easier if I just send individual additions to
> your patches.
>

Ack, thanks for reviewing patches and giving valuable feedback. Sending new patchset series within sometime.

    Regards,
    Het Gala