Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Het Gala <het.gala@nutanix.com>]

On 15/05/23 8:46 pm, Het Gala wrote:
>
> On 15/05/23 8:16 pm, Juan Quintela wrote:
>> Het Gala <het.gala@nutanix.com> wrote:
>>> On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
>>>> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
>>>>> Socket transport backend for 'migrate'/'migrate-incoming' QAPIs 
>>>>> accept
>>>>> new wire protocol of MigrateAddress struct.
>>>>>
>>>>> It is achived by parsing 'uri' string and storing migration 
>>>>> parameters
>>>>> required for socket connection into well defined SocketAddress 
>>>>> struct.
>>>>>
>>>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>>>> ---
>>>>>    migration/exec.c      |  4 ++--
>>>>>    migration/exec.h      |  4 ++++
>>>>>    migration/migration.c | 44 
>>>>> +++++++++++++++++++++++++++++++------------
>>>>>    migration/socket.c    | 34 +++++----------------------------
>>>>>    migration/socket.h    |  7 ++++---
>>>>>    5 files changed, 47 insertions(+), 46 deletions(-)
>>>>>
>>>>> diff --git a/migration/exec.c b/migration/exec.c
>>>>> index 2bf882bbe1..c4a3293246 100644
>>>>> --- a/migration/exec.c
>>>>> +++ b/migration/exec.c
>>>>> @@ -27,7 +27,6 @@
>>>>>    #include "qemu/cutils.h"
>>>>>      #ifdef WIN32
>>>>> -const char *exec_get_cmd_path(void);
>>>>>    const char *exec_get_cmd_path(void)
Maintainers please advice. If I want to see thatthe build is proper, how 
to enable WIN32 support on a CentOS guest operating system (what all 
dependencies to install, what to configure for a build which supports 
WIN32) ? Any pointers ?
>>> Even this, I will shift it to the 2nd patch, where I need to move
>>> exec_get_cmd_path() out accross other file (migration.c).
>> great.
>>
>>>>>    {
>>>>>        g_autofree char *detected_path = g_new(char, MAX_PATH);
>>>>> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>>>>>    }
>>>>>    #endif
>>>>>    -void exec_start_outgoing_migration(MigrationState *s, const char
>>>>> *command, Error **errp)
>>>>> +void exec_start_outgoing_migration(MigrationState *s, const char 
>>>>> *command,
>>>>> +                                   Error **errp)
>>>>>    {
>>>>>        QIOChannel *ioc;
>>> Sure, Juan. Will change this in the 2nd patch itself instead of
>>> here. I am not very convinved why should we have a different patch all
>>> together for this, because we are just using this code outside this
>>> file in my opinion? But if you still think so, I can make a different
>>> patch for that.
>> It is up to you.
>
> For now, I will keep it with 2nd patch. If any other maintainer also 
> thinks that it should be a separate patch all together of windows 
> support for exec, I will make a new patch for that. But thankyou for 
> your suggestion 😁
>
>>> Juan, I get your point. But I think, we won't be needing local_err at
>>> all, if I use g_autoptr for 'channel' and 'saddr' is a part of
>>> 'channel'. Let me have a v2 patchset and if it is still not
>>> convinving, we can have a discussion on this.
>>>> THis leaks 'channel', and free's 'saddr' which actually  belongs
>>>> to channel.
>>>>
>>>> With my comments on the previous patch suggesting g_autoptr for
>>>> 'channel', we don't need any free calls for 'saddr' or 'channel'.
>>> Right. With g_autoptr used for freeing 'channel' in last patch, we
>>> wont have to worry about freeing 'saddr' at all. Thanks Daniel
>>>
>>> if (local_err) {
>>>      qapi_free_SocketAddress(saddr);
>>>      error_propagate(errp, local_err);
>>>      return;
>>> }
>>> And after changing the position for assigning 'saddr' and using
>>> g_autoptr for 'channel' I believe we can get rid of 'local_error'
>>> variable too and replace it with 'errp'. Please suggest if I am
>>> missing something here. TIA!
>> great.  That is much better.
> Ack.
>>>>>    -void socket_start_outgoing_migration(MigrationState *s,
>>>>> -                                     const char *str,
>>>>> -                                     Error **errp)
>>>>> -{
>>>>> -    Error *err = NULL;
>>>>> -    SocketAddress *saddr = socket_parse(str, &err);
>>>>> -    if (!err) {
>>>>> -        socket_start_outgoing_migration_internal(s, saddr, &err);
>>>>> -    }
>>>>> -    error_propagate(errp, err);
>>>>> -}
>>>>> -
>>> Actually Juan. I don't need this function at all, because parsing of
>>> uri into socketAddress using socket_parse is already done. So there is
>>> no use of having this function in the first place, so I decided to
>>> delete this fucntion all together. Same with incoming function.
>> What I mean is that this code was already there.  And it was doing it
>> wrong.  The parts that I corrected you were using this pattern, chcking
>> that err was NULL, intsead of cheking that saddr is NULL.
> Yes, I get your point. But that function is useless if socket_parse() 
> function is not needed. So have omitted the function all together as 
> socket parsing is already done in earlier patches.
>> Later, Juan.
> Regards,
> Het Gala