[PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below

[PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below

[Het Gala]

'object' can return NULL if there is no socket-address, such as with a
file migration. Then the visitor code below fails and the test crashes.

Ignore and return NULL when socket-address is missing in the reply so
we don't break future tests that use a non-socket type.

Suggested-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/migration-helpers.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..fb7156f09a 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState *who)
     QObject *object;
 
     rsp = migrate_query(who);
+
+    if (!qdict_haskey(rsp, "socket-address")) {
+        return NULL;
+    }
     object = qdict_get(rsp, "socket-address");
 
     iv = qobject_input_visitor_new(object);
-- 
2.22.3

[PATCH 2/2] tests/qtest/migration: Fix typo for vsock in SocketAddress_to_str

[Het Gala]

Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/migration-helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index fb7156f09a..651c6c555a 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -42,7 +42,7 @@ static char *SocketAddress_to_str(SocketAddress *addr)
     case SOCKET_ADDRESS_TYPE_FD:
         return g_strdup_printf("fd:%s", addr->u.fd.str);
     case SOCKET_ADDRESS_TYPE_VSOCK:
-        return g_strdup_printf("tcp:%s:%s",
+        return g_strdup_printf("vsock:%s:%s",
                                addr->u.vsock.cid,
                                addr->u.vsock.port);
     default:
-- 
2.22.3

Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below

[Het Gala]

FYI: This 2 patches are rebased on top of another 
(tests/qtest/migration: Add tests for introducing 'channels' argument in 
migrate QAPIs) series.
Can find the build for both the patches here: 
https://gitlab.com/galahet/Qemu/-/pipelines/1219841944

On 20/03/24 2:18 am, Het Gala wrote:
> 'object' can return NULL if there is no socket-address, such as with a
> file migration. Then the visitor code below fails and the test crashes.
>
> Ignore and return NULL when socket-address is missing in the reply so
> we don't break future tests that use a non-socket type.
>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>   tests/qtest/migration-helpers.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index b2a90469fb..fb7156f09a 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState *who)
>       QObject *object;
>   
>       rsp = migrate_query(who);
> +
> +    if (!qdict_haskey(rsp, "socket-address")) {
> +        return NULL;
> +    }
>       object = qdict_get(rsp, "socket-address");
>   
>       iv = qobject_input_visitor_new(object);
Regards,
Het Gala

Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below

[Peter Xu]

On Tue, Mar 19, 2024 at 08:48:39PM +0000, Het Gala wrote:
> 'object' can return NULL if there is no socket-address, such as with a
> file migration. Then the visitor code below fails and the test crashes.
> 
> Ignore and return NULL when socket-address is missing in the reply so
> we don't break future tests that use a non-socket type.

Hmm, this patch isn't as clear to me.  Even if this can return NULL now,
it'll soon crash at some later point, no?

IMHO such patch is more suitable to be included in the same patch where
such new tests will be introduced, then we're addressing some real test
code changes that will work, rather than worrying on what will happen in
the future (and as I mentioned, i don't think it fully resolved that either..)

Thanks,

> 
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  tests/qtest/migration-helpers.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index b2a90469fb..fb7156f09a 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState *who)
>      QObject *object;
>  
>      rsp = migrate_query(who);
> +
> +    if (!qdict_haskey(rsp, "socket-address")) {
> +        return NULL;
> +    }
>      object = qdict_get(rsp, "socket-address");
>  
>      iv = qobject_input_visitor_new(object);
> -- 
> 2.22.3
> 

-- 
Peter Xu

Re: [PATCH 2/2] tests/qtest/migration: Fix typo for vsock in SocketAddress_to_str

[Peter Xu]

On Tue, Mar 19, 2024 at 08:48:40PM +0000, Het Gala wrote:
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  tests/qtest/migration-helpers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index fb7156f09a..651c6c555a 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -42,7 +42,7 @@ static char *SocketAddress_to_str(SocketAddress *addr)
>      case SOCKET_ADDRESS_TYPE_FD:
>          return g_strdup_printf("fd:%s", addr->u.fd.str);
>      case SOCKET_ADDRESS_TYPE_VSOCK:
> -        return g_strdup_printf("tcp:%s:%s",
> +        return g_strdup_printf("vsock:%s:%s",
>                                 addr->u.vsock.cid,
>                                 addr->u.vsock.port);
>      default:

Queued for 9.1, thanks.

-- 
Peter Xu

Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below

[Het Gala]

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


On 20/03/24 3:27 am, Peter Xu wrote:
> On Tue, Mar 19, 2024 at 08:48:39PM +0000, Het Gala wrote:
>> 'object' can return NULL if there is no socket-address, such as with a
>> file migration. Then the visitor code below fails and the test crashes.
>>
>> Ignore and return NULL when socket-address is missing in the reply so
>> we don't break future tests that use a non-socket type.
> Hmm, this patch isn't as clear to me.  Even if this can return NULL now,
> it'll soon crash at some later point, no?
It won't crash IMO, the next function SocketAddress_to_str for a non-socket
type would return an proper error ?
> IMHO such patch is more suitable to be included in the same patch where
> such new tests will be introduced, then we're addressing some real test
> code changes that will work, rather than worrying on what will happen in
> the future (and as I mentioned, i don't think it fully resolved that either..)
>
> Thanks,
Maybe, Fabiano can pick this patch, and add along file migration qtests 
patch ?
>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>> ---
>>   tests/qtest/migration-helpers.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index b2a90469fb..fb7156f09a 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState *who)
>>       QObject *object;
>>   
>>       rsp = migrate_query(who);
>> +
>> +    if (!qdict_haskey(rsp, "socket-address")) {
>> +        return NULL;
>> +    }
>>       object = qdict_get(rsp, "socket-address");
>>   
>>       iv = qobject_input_visitor_new(object);
>> -- 
>> 2.22.3
>>
Regards,
Het Gala

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

Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below

[Peter Xu]

On Wed, Mar 20, 2024 at 10:47:18AM +0530, Het Gala wrote:
> It won't crash IMO, the next function SocketAddress_to_str for a non-socket
> type would return an proper error ?

Both SocketAddress_to_str() and SocketAddress_to_qdict() will crash with a
NULL pointer passed in right now.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> On 20/03/24 3:27 am, Peter Xu wrote:
>> On Tue, Mar 19, 2024 at 08:48:39PM +0000, Het Gala wrote:
>>> 'object' can return NULL if there is no socket-address, such as with a
>>> file migration. Then the visitor code below fails and the test crashes.
>>>
>>> Ignore and return NULL when socket-address is missing in the reply so
>>> we don't break future tests that use a non-socket type.
>> Hmm, this patch isn't as clear to me.  Even if this can return NULL now,
>> it'll soon crash at some later point, no?
> It won't crash IMO, the next function SocketAddress_to_str for a non-socket
> type would return an proper error ?
>> IMHO such patch is more suitable to be included in the same patch where
>> such new tests will be introduced, then we're addressing some real test
>> code changes that will work, rather than worrying on what will happen in
>> the future (and as I mentioned, i don't think it fully resolved that either..)
>>
>> Thanks,
> Maybe, Fabiano can pick this patch, and add along file migration qtests 
> patch ?

Yep.

>>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>> ---
>>>   tests/qtest/migration-helpers.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>> index b2a90469fb..fb7156f09a 100644
>>> --- a/tests/qtest/migration-helpers.c
>>> +++ b/tests/qtest/migration-helpers.c
>>> @@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState *who)
>>>       QObject *object;
>>>   
>>>       rsp = migrate_query(who);
>>> +
>>> +    if (!qdict_haskey(rsp, "socket-address")) {
>>> +        return NULL;
>>> +    }
>>>       object = qdict_get(rsp, "socket-address");
>>>   
>>>       iv = qobject_input_visitor_new(object);
>>> -- 
>>> 2.22.3
>>>
> Regards,
> Het Gala

Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below

[Het Gala]

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


On 20/03/24 6:49 pm, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> On 20/03/24 3:27 am, Peter Xu wrote:
>>> On Tue, Mar 19, 2024 at 08:48:39PM +0000, Het Gala wrote:
>>>> 'object' can return NULL if there is no socket-address, such as with a
>>>> file migration. Then the visitor code below fails and the test crashes.
>>>>
>>>> Ignore and return NULL when socket-address is missing in the reply so
>>>> we don't break future tests that use a non-socket type.
>>> Hmm, this patch isn't as clear to me.  Even if this can return NULL now,
>>> it'll soon crash at some later point, no?
>> It won't crash IMO, the next function SocketAddress_to_str for a non-socket
>> type would return an proper error ?
>>> IMHO such patch is more suitable to be included in the same patch where
>>> such new tests will be introduced, then we're addressing some real test
>>> code changes that will work, rather than worrying on what will happen in
>>> the future (and as I mentioned, i don't think it fully resolved that either..)
>>>
>>> Thanks,
>> Maybe, Fabiano can pick this patch, and add along file migration qtests
>> patch ?
> Yep.
Thanks
>>>> @@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState *who)
>>>>        QObject *object;
>>>>    
>>>>        rsp = migrate_query(who);
>>>> +
>>>> +    if (!qdict_haskey(rsp, "socket-address")) {
>>>> +        return NULL;
>>>> +    }
>>>>        object = qdict_get(rsp, "socket-address");
>>>>    
>>>>        iv = qobject_input_visitor_new(object);
>>>> -- 
>>>> 2.22.3
>>>>
>> Regards,
>> Het Gala

Regards,
Het Gala

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