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

[Juan Quintela]

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)
> 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.


> 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.

>>>   -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.

Later, Juan.