| Message ID | 20240312202634.63349-4-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests/qtest/migration: Add tests for introducing 'channels' argument in migrate QAPIs | expand |
Het Gala <het.gala@nutanix.com> writes: > Refactor migrate_get_socket_address to internally utilize 'socket-address' > parameter, reducing redundancy in the function definition. > > migrate_get_socket_address implicitly converts SocketAddress into str. > Move migrate_get_socket_address inside migrate_get_connect_uri which > should return the uri string instead. > > Signed-off-by: Het Gala <het.gala@nutanix.com> > Suggested-by: Fabiano Rosas <farosas@suse.de> > Reviewed-by: Fabiano Rosas <farosas@suse.de> > --- > tests/qtest/migration-helpers.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c > index 3e8c19c4de..8806dc841e 100644 > --- a/tests/qtest/migration-helpers.c > +++ b/tests/qtest/migration-helpers.c > @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr) > } > } > > -static char * > -migrate_get_socket_address(QTestState *who, const char *parameter) > +static SocketAddress *migrate_get_socket_address(QTestState *who) > { > QDict *rsp; > - char *result; > SocketAddressList *addrs; > + SocketAddress *addr; > Visitor *iv = NULL; > QObject *object; > > rsp = migrate_query(who); > - object = qdict_get(rsp, parameter); > + object = qdict_get(rsp, "socket-address"); Just a heads up, none of what I'm about to say applies to current master. This can return NULL if there is no socket-address, such as with a file migration. Then the visitor code below just barfs. It would be nice if we touched this up eventually. I only noticed this because I was fiddling with the file migration API and this series helped me a lot to test my changes. So thanks for that, Het. Another point is: we really need to encourage people to write tests using the new channels API. I added the FileMigrationArgs with the 'offset' as a required parameter, not even knowing optional parameters were a thing. So it's obviously not enough to write support for the new API if no tests ever touch it. > > iv = qobject_input_visitor_new(object); > visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort); > + addr = addrs->value; > visit_free(iv); > > - /* we are only using a single address */ > - result = SocketAddress_to_str(addrs->value); > - > - qapi_free_SocketAddressList(addrs); > qobject_unref(rsp); > - return result; > + return addr; > +} > + > +static char * > +migrate_get_connect_uri(QTestState *who) > +{ > + SocketAddress *addrs; > + char *connect_uri; > + > + addrs = migrate_get_socket_address(who); > + connect_uri = SocketAddress_to_str(addrs); > + > + qapi_free_SocketAddress(addrs); > + return connect_uri; > } > > bool migrate_watch_for_events(QTestState *who, const char *name, > @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri, > > g_assert(!qdict_haskey(args, "uri")); > if (!uri) { > - connect_uri = migrate_get_socket_address(to, "socket-address"); > + connect_uri = migrate_get_connect_uri(to); > } > qdict_put_str(args, "uri", uri ? uri : connect_uri);
On 15/03/24 6:28 pm, Fabiano Rosas wrote: > Het Gala<het.gala@nutanix.com> writes: > >> Refactor migrate_get_socket_address to internally utilize 'socket-address' >> parameter, reducing redundancy in the function definition. >> >> migrate_get_socket_address implicitly converts SocketAddress into str. >> Move migrate_get_socket_address inside migrate_get_connect_uri which >> should return the uri string instead. >> >> Signed-off-by: Het Gala<het.gala@nutanix.com> >> Suggested-by: Fabiano Rosas<farosas@suse.de> >> Reviewed-by: Fabiano Rosas<farosas@suse.de> >> --- >> tests/qtest/migration-helpers.c | 29 +++++++++++++++++++---------- >> 1 file changed, 19 insertions(+), 10 deletions(-) >> >> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c >> index 3e8c19c4de..8806dc841e 100644 >> --- a/tests/qtest/migration-helpers.c >> +++ b/tests/qtest/migration-helpers.c >> @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr) >> } >> } >> >> -static char * >> -migrate_get_socket_address(QTestState *who, const char *parameter) >> +static SocketAddress *migrate_get_socket_address(QTestState *who) >> { >> QDict *rsp; >> - char *result; >> SocketAddressList *addrs; >> + SocketAddress *addr; >> Visitor *iv = NULL; >> QObject *object; >> >> rsp = migrate_query(who); >> - object = qdict_get(rsp, parameter); >> + object = qdict_get(rsp, "socket-address"); > Just a heads up, none of what I'm about to say applies to current > master. > > This can return NULL if there is no socket-address, such as with a file > migration. Then the visitor code below just barfs. It would be nice if > we touched this up eventually. Yes. I agree this is not full proof solution and covers for all the cases. It would only for 'socket-address'. Could you elaborate on what other than socket-address the QObject can have ? > I only noticed this because I was fiddling with the file migration API > and this series helped me a lot to test my changes. So thanks for that, > Het. > > Another point is: we really need to encourage people to write tests > using the new channels API. I added the FileMigrationArgs with the > 'offset' as a required parameter, not even knowing optional parameters > were a thing. So it's obviously not enough to write support for the new > API if no tests ever touch it. Yes, definitely we need more tests with the new channels API to test other than just tcp connection. I could give a try for vsock and unix with the new QAPI syntax, and add some tests. I also wanted to bring in attention that, this solution I what i feel is also not complete. If we are using new channel syntax for migrate_qmp, then the same syntax should also be used for migrate_qmp_incoming. But we haven't touch that, and it still prints the old syntax. We might need a lot changes in design maybe to incorporate that too in new tests totally with the new syntax. Another thing that you also noted down while discussing on the patches that we should have a standard pattern on how to define the migration tests. Even that would be helpful for the users, on how to add new tests, where to add new tests in the file, and which test is needed to run if a specific change needs to be tested. >> >> iv = qobject_input_visitor_new(object); >> visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort); >> + addr = addrs->value; >> visit_free(iv); >> >> - /* we are only using a single address */ >> - result = SocketAddress_to_str(addrs->value); >> - >> - qapi_free_SocketAddressList(addrs); >> qobject_unref(rsp); >> - return result; >> + return addr; >> +} >> + >> +static char * >> +migrate_get_connect_uri(QTestState *who) >> +{ >> + SocketAddress *addrs; >> + char *connect_uri; >> + >> + addrs = migrate_get_socket_address(who); >> + connect_uri = SocketAddress_to_str(addrs); >> + >> + qapi_free_SocketAddress(addrs); >> + return connect_uri; >> } >> >> bool migrate_watch_for_events(QTestState *who, const char *name, >> @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri, >> >> g_assert(!qdict_haskey(args, "uri")); >> if (!uri) { >> - connect_uri = migrate_get_socket_address(to, "socket-address"); >> + connect_uri = migrate_get_connect_uri(to); >> } >> qdict_put_str(args, "uri", uri ? uri : connect_uri); Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 15/03/24 6:28 pm, Fabiano Rosas wrote: >> Het Gala<het.gala@nutanix.com> writes: >> >>> Refactor migrate_get_socket_address to internally utilize 'socket-address' >>> parameter, reducing redundancy in the function definition. >>> >>> migrate_get_socket_address implicitly converts SocketAddress into str. >>> Move migrate_get_socket_address inside migrate_get_connect_uri which >>> should return the uri string instead. >>> >>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>> Suggested-by: Fabiano Rosas<farosas@suse.de> >>> Reviewed-by: Fabiano Rosas<farosas@suse.de> >>> --- >>> tests/qtest/migration-helpers.c | 29 +++++++++++++++++++---------- >>> 1 file changed, 19 insertions(+), 10 deletions(-) >>> >>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c >>> index 3e8c19c4de..8806dc841e 100644 >>> --- a/tests/qtest/migration-helpers.c >>> +++ b/tests/qtest/migration-helpers.c >>> @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr) >>> } >>> } >>> >>> -static char * >>> -migrate_get_socket_address(QTestState *who, const char *parameter) >>> +static SocketAddress *migrate_get_socket_address(QTestState *who) >>> { >>> QDict *rsp; >>> - char *result; >>> SocketAddressList *addrs; >>> + SocketAddress *addr; >>> Visitor *iv = NULL; >>> QObject *object; >>> >>> rsp = migrate_query(who); >>> - object = qdict_get(rsp, parameter); >>> + object = qdict_get(rsp, "socket-address"); >> Just a heads up, none of what I'm about to say applies to current >> master. >> >> This can return NULL if there is no socket-address, such as with a file >> migration. Then the visitor code below just barfs. It would be nice if >> we touched this up eventually. > > Yes. I agree this is not full proof solution and covers for all the cases. > It would only for 'socket-address'. Could you elaborate on what other than > socket-address the QObject can have ? I can just not have the socket-address, AFAICS. We'd just need to not crash if that's the case. > >> I only noticed this because I was fiddling with the file migration API >> and this series helped me a lot to test my changes. So thanks for that, >> Het. >> >> Another point is: we really need to encourage people to write tests >> using the new channels API. I added the FileMigrationArgs with the >> 'offset' as a required parameter, not even knowing optional parameters >> were a thing. So it's obviously not enough to write support for the new >> API if no tests ever touch it. > Yes, definitely we need more tests with the new channels API to test other > than just tcp connection. I could give a try for vsock and unix with the > new QAPI syntax, and add some tests. > > I also wanted to bring in attention that, this solution I what i feel is > also > not complete. If we are using new channel syntax for migrate_qmp, then the > same syntax should also be used for migrate_qmp_incoming. But we haven't > touch that, and it still prints the old syntax. We might need a lot changes > in design maybe to incorporate that too in new tests totally with the new > syntax. Adding migrate_qmp_incoming support should be relatively simple. You had patches for that in another version, no? > > Another thing that you also noted down while discussing on the patches that > we should have a standard pattern on how to define the migration tests. Even > that would be helpful for the users, on how to add new tests, where to add > new tests in the file, and which test is needed to run if a specific change > needs to be tested. > >>> >>> iv = qobject_input_visitor_new(object); >>> visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort); >>> + addr = addrs->value; >>> visit_free(iv); >>> >>> - /* we are only using a single address */ >>> - result = SocketAddress_to_str(addrs->value); >>> - >>> - qapi_free_SocketAddressList(addrs); >>> qobject_unref(rsp); >>> - return result; >>> + return addr; >>> +} >>> + >>> +static char * >>> +migrate_get_connect_uri(QTestState *who) >>> +{ >>> + SocketAddress *addrs; >>> + char *connect_uri; >>> + >>> + addrs = migrate_get_socket_address(who); >>> + connect_uri = SocketAddress_to_str(addrs); >>> + >>> + qapi_free_SocketAddress(addrs); >>> + return connect_uri; >>> } >>> >>> bool migrate_watch_for_events(QTestState *who, const char *name, >>> @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri, >>> >>> g_assert(!qdict_haskey(args, "uri")); >>> if (!uri) { >>> - connect_uri = migrate_get_socket_address(to, "socket-address"); >>> + connect_uri = migrate_get_connect_uri(to); >>> } >>> qdict_put_str(args, "uri", uri ? uri : connect_uri); > > Regards, > Het Gala
On 18/03/24 7:46 pm, Fabiano Rosas wrote: > Het Gala<het.gala@nutanix.com> writes: > >> On 15/03/24 6:28 pm, Fabiano Rosas wrote: >>> Het Gala<het.gala@nutanix.com> writes: >>> >>>> Refactor migrate_get_socket_address to internally utilize 'socket-address' >>>> parameter, reducing redundancy in the function definition. >>>> >>>> migrate_get_socket_address implicitly converts SocketAddress into str. >>>> Move migrate_get_socket_address inside migrate_get_connect_uri which >>>> should return the uri string instead. >>>> >>>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>>> Suggested-by: Fabiano Rosas<farosas@suse.de> >>>> Reviewed-by: Fabiano Rosas<farosas@suse.de> >>>> --- >>>> tests/qtest/migration-helpers.c | 29 +++++++++++++++++++---------- >>>> 1 file changed, 19 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c >>>> index 3e8c19c4de..8806dc841e 100644 >>>> --- a/tests/qtest/migration-helpers.c >>>> +++ b/tests/qtest/migration-helpers.c >>>> @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr) >>>> } >>>> } >>>> >>>> -static char * >>>> -migrate_get_socket_address(QTestState *who, const char *parameter) >>>> +static SocketAddress *migrate_get_socket_address(QTestState *who) >>>> { >>>> QDict *rsp; >>>> - char *result; >>>> SocketAddressList *addrs; >>>> + SocketAddress *addr; >>>> Visitor *iv = NULL; >>>> QObject *object; >>>> >>>> rsp = migrate_query(who); >>>> - object = qdict_get(rsp, parameter); >>>> + object = qdict_get(rsp, "socket-address"); >>> Just a heads up, none of what I'm about to say applies to current >>> master. >>> >>> This can return NULL if there is no socket-address, such as with a file >>> migration. Then the visitor code below just barfs. It would be nice if >>> we touched this up eventually. >> Yes. I agree this is not full proof solution and covers for all the cases. >> It would only for 'socket-address'. Could you elaborate on what other than >> socket-address the QObject can have ? > I can just not have the socket-address, AFAICS. We'd just need to not > crash if that's the case. value: { "status": "setup", "socket-address": [ { "port": "46213", "ipv4": true, "host": "127.0.0.1", "type": "inet" } ] } Okay, I understood your ask here. This is what gets printed from the QDict. Let me raise a patch to return with a message if the QDict does not have key with 'socket-address'. This would prevent the crash later on. I wanted to know what other than "socket-address" key can he QDict give us because, if that's the case, for other than socket migration, then we can make this function more generic rather than having it as 'migrate_get_socket_address' >>> I only noticed this because I was fiddling with the file migration API >>> and this series helped me a lot to test my changes. So thanks for that, >>> Het. >>> >>> Another point is: we really need to encourage people to write tests >>> using the new channels API. I added the FileMigrationArgs with the >>> 'offset' as a required parameter, not even knowing optional parameters >>> were a thing. So it's obviously not enough to write support for the new >>> API if no tests ever touch it. >> Yes, definitely we need more tests with the new channels API to test other >> than just tcp connection. I could give a try for vsock and unix with the >> new QAPI syntax, and add some tests. >> >> I also wanted to bring in attention that, this solution I what i feel is >> also >> not complete. If we are using new channel syntax for migrate_qmp, then the >> same syntax should also be used for migrate_qmp_incoming. But we haven't >> touch that, and it still prints the old syntax. We might need a lot changes >> in design maybe to incorporate that too in new tests totally with the new >> syntax. > Adding migrate_qmp_incoming support should be relatively simple. You had > patches for that in another version, no? No Fabiano, what I meant was, in migration-test.c, change in migrate_incoming_qmp would need to change the callback function and ultimately change all the callback handlers ? In that sense, it would require a big change ? Inside the migrate_incoming_qmp function, adding implementation for channels is same as other migrate_* function. >> Another thing that you also noted down while discussing on the patches that >> we should have a standard pattern on how to define the migration tests. Even >> that would be helpful for the users, on how to add new tests, where to add >> new tests in the file, and which test is needed to run if a specific change >> needs to be tested. >> >>>> >>>> iv = qobject_input_visitor_new(object); >>>> visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort); >>>> + addr = addrs->value; >>>> visit_free(iv); >>>> >>>> - /* we are only using a single address */ >>>> - result = SocketAddress_to_str(addrs->value); >>>> - >>>> - qapi_free_SocketAddressList(addrs); >>>> qobject_unref(rsp); >>>> - return result; >>>> + return addr; >>>> +} >>>> + >>>> +static char * >>>> +migrate_get_connect_uri(QTestState *who) >>>> +{ >>>> + SocketAddress *addrs; >>>> + char *connect_uri; >>>> + >>>> + addrs = migrate_get_socket_address(who); >>>> + connect_uri = SocketAddress_to_str(addrs); >>>> + >>>> + qapi_free_SocketAddress(addrs); >>>> + return connect_uri; >>>> } >>>> >>>> bool migrate_watch_for_events(QTestState *who, const char *name, >>>> @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri, >>>> >>>> g_assert(!qdict_haskey(args, "uri")); >>>> if (!uri) { >>>> - connect_uri = migrate_get_socket_address(to, "socket-address"); >>>> + connect_uri = migrate_get_connect_uri(to); >>>> } >>>> qdict_put_str(args, "uri", uri ? uri : connect_uri); >> Regards, >> Het Gala Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 18/03/24 7:46 pm, Fabiano Rosas wrote: >> Het Gala<het.gala@nutanix.com> writes: >> >>> On 15/03/24 6:28 pm, Fabiano Rosas wrote: >>>> Het Gala<het.gala@nutanix.com> writes: >>>> >>>>> Refactor migrate_get_socket_address to internally utilize 'socket-address' >>>>> parameter, reducing redundancy in the function definition. >>>>> >>>>> migrate_get_socket_address implicitly converts SocketAddress into str. >>>>> Move migrate_get_socket_address inside migrate_get_connect_uri which >>>>> should return the uri string instead. >>>>> >>>>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>>>> Suggested-by: Fabiano Rosas<farosas@suse.de> >>>>> Reviewed-by: Fabiano Rosas<farosas@suse.de> >>>>> --- >>>>> tests/qtest/migration-helpers.c | 29 +++++++++++++++++++---------- >>>>> 1 file changed, 19 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c >>>>> index 3e8c19c4de..8806dc841e 100644 >>>>> --- a/tests/qtest/migration-helpers.c >>>>> +++ b/tests/qtest/migration-helpers.c >>>>> @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr) >>>>> } >>>>> } >>>>> >>>>> -static char * >>>>> -migrate_get_socket_address(QTestState *who, const char *parameter) >>>>> +static SocketAddress *migrate_get_socket_address(QTestState *who) >>>>> { >>>>> QDict *rsp; >>>>> - char *result; >>>>> SocketAddressList *addrs; >>>>> + SocketAddress *addr; >>>>> Visitor *iv = NULL; >>>>> QObject *object; >>>>> >>>>> rsp = migrate_query(who); >>>>> - object = qdict_get(rsp, parameter); >>>>> + object = qdict_get(rsp, "socket-address"); >>>> Just a heads up, none of what I'm about to say applies to current >>>> master. >>>> >>>> This can return NULL if there is no socket-address, such as with a file >>>> migration. Then the visitor code below just barfs. It would be nice if >>>> we touched this up eventually. >>> Yes. I agree this is not full proof solution and covers for all the cases. >>> It would only for 'socket-address'. Could you elaborate on what other than >>> socket-address the QObject can have ? >> I can just not have the socket-address, AFAICS. We'd just need to not >> crash if that's the case. > > value: { > "status": "setup", > "socket-address": [ > { > "port": "46213", > "ipv4": true, > "host": "127.0.0.1", > "type": "inet" > } > ] > } > > Okay, I understood your ask here. This is what gets printed from the QDict. > Let me raise a patch to return with a message if the QDict does not have key > with 'socket-address'. This would prevent the crash later on. > > I wanted to know what other than "socket-address" key can he QDict give us > because, if that's the case, for other than socket migration, then we can > make this function more generic rather than having it as > 'migrate_get_socket_address' For now, there's nothing else. Let's just ignore when socket-address is missing in the reply so we don't break future tests that use a non-socket type. >>>> I only noticed this because I was fiddling with the file migration API >>>> and this series helped me a lot to test my changes. So thanks for that, >>>> Het. >>>> >>>> Another point is: we really need to encourage people to write tests >>>> using the new channels API. I added the FileMigrationArgs with the >>>> 'offset' as a required parameter, not even knowing optional parameters >>>> were a thing. So it's obviously not enough to write support for the new >>>> API if no tests ever touch it. >>> Yes, definitely we need more tests with the new channels API to test other >>> than just tcp connection. I could give a try for vsock and unix with the >>> new QAPI syntax, and add some tests. >>> >>> I also wanted to bring in attention that, this solution I what i feel is >>> also >>> not complete. If we are using new channel syntax for migrate_qmp, then the >>> same syntax should also be used for migrate_qmp_incoming. But we haven't >>> touch that, and it still prints the old syntax. We might need a lot changes >>> in design maybe to incorporate that too in new tests totally with the new >>> syntax. >> Adding migrate_qmp_incoming support should be relatively simple. You had >> patches for that in another version, no? > No Fabiano, what I meant was, in migration-test.c, change in > migrate_incoming_qmp > would need to change the callback function and ultimately change all the > callback > handlers ? In that sense, it would require a big change ? > Inside the migrate_incoming_qmp function, adding implementation for > channels is > same as other migrate_* function. You could add the parameter to migrate_incoming_qmp and use NULL when calling. The callbacks don't need to be changed. When we add more tests then we'd alter the callbacks accordingly. I might convert the file tests soon, you can leave that part to me if you want. >>> Another thing that you also noted down while discussing on the patches that >>> we should have a standard pattern on how to define the migration tests. Even >>> that would be helpful for the users, on how to add new tests, where to add >>> new tests in the file, and which test is needed to run if a specific change >>> needs to be tested. >>> >>>>> >>>>> iv = qobject_input_visitor_new(object); >>>>> visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort); >>>>> + addr = addrs->value; >>>>> visit_free(iv); >>>>> >>>>> - /* we are only using a single address */ >>>>> - result = SocketAddress_to_str(addrs->value); >>>>> - >>>>> - qapi_free_SocketAddressList(addrs); >>>>> qobject_unref(rsp); >>>>> - return result; >>>>> + return addr; >>>>> +} >>>>> + >>>>> +static char * >>>>> +migrate_get_connect_uri(QTestState *who) >>>>> +{ >>>>> + SocketAddress *addrs; >>>>> + char *connect_uri; >>>>> + >>>>> + addrs = migrate_get_socket_address(who); >>>>> + connect_uri = SocketAddress_to_str(addrs); >>>>> + >>>>> + qapi_free_SocketAddress(addrs); >>>>> + return connect_uri; >>>>> } >>>>> >>>>> bool migrate_watch_for_events(QTestState *who, const char *name, >>>>> @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri, >>>>> >>>>> g_assert(!qdict_haskey(args, "uri")); >>>>> if (!uri) { >>>>> - connect_uri = migrate_get_socket_address(to, "socket-address"); >>>>> + connect_uri = migrate_get_connect_uri(to); >>>>> } >>>>> qdict_put_str(args, "uri", uri ? uri : connect_uri); >>> Regards, >>> Het Gala > Regards, > Het Gala
On 20/03/24 12:33 am, Fabiano Rosas wrote: > Het Gala<het.gala@nutanix.com> writes: > >> On 18/03/24 7:46 pm, Fabiano Rosas wrote: >>> Het Gala<het.gala@nutanix.com> writes: >>> >>>> On 15/03/24 6:28 pm, Fabiano Rosas wrote: >>>>> Het Gala<het.gala@nutanix.com> writes: >>>>> >>>>>> Refactor migrate_get_socket_address to internally utilize 'socket-address' >>>>>> parameter, reducing redundancy in the function definition. >>>>>> >>>>>> migrate_get_socket_address implicitly converts SocketAddress into str. >>>>>> Move migrate_get_socket_address inside migrate_get_connect_uri which >>>>>> should return the uri string instead. >>>>>> >>>>>> -static char * >>>>>> -migrate_get_socket_address(QTestState *who, const char *parameter) >>>>>> +static SocketAddress *migrate_get_socket_address(QTestState *who) >>>>>> { >>>>>> QDict *rsp; >>>>>> - char *result; >>>>>> SocketAddressList *addrs; >>>>>> + SocketAddress *addr; >>>>>> Visitor *iv = NULL; >>>>>> QObject *object; >>>>>> >>>>>> rsp = migrate_query(who); >>>>>> - object = qdict_get(rsp, parameter); >>>>>> + object = qdict_get(rsp, "socket-address"); >>>>> Just a heads up, none of what I'm about to say applies to current >>>>> master. >>>>> >>>>> This can return NULL if there is no socket-address, such as with a file >>>>> migration. Then the visitor code below just barfs. It would be nice if >>>>> we touched this up eventually. >>>> Yes. I agree this is not full proof solution and covers for all the cases. >>>> It would only for 'socket-address'. Could you elaborate on what other than >>>> socket-address the QObject can have ? >>> I can just not have the socket-address, AFAICS. We'd just need to not >>> crash if that's the case. >> value: { >> "status": "setup", >> "socket-address": [ >> { >> "port": "46213", >> "ipv4": true, >> "host": "127.0.0.1", >> "type": "inet" >> } >> ] >> } >> >> Okay, I understood your ask here. This is what gets printed from the QDict. >> Let me raise a patch to return with a message if the QDict does not have key >> with 'socket-address'. This would prevent the crash later on. >> >> I wanted to know what other than "socket-address" key can he QDict give us >> because, if that's the case, for other than socket migration, then we can >> make this function more generic rather than having it as >> 'migrate_get_socket_address' > For now, there's nothing else. Let's just ignore when socket-address is > missing in the reply so we don't break future tests that use a > non-socket type. Okay, Done. Can find the build here: https://gitlab.com/galahet/Qemu/-/pipelines/1219841944 >>>>> I only noticed this because I was fiddling with the file migration API >>>>> and this series helped me a lot to test my changes. So thanks for that, >>>>> Het. >>>>> >>>>> Another point is: we really need to encourage people to write tests >>>>> using the new channels API. I added the FileMigrationArgs with the >>>>> 'offset' as a required parameter, not even knowing optional parameters >>>>> were a thing. So it's obviously not enough to write support for the new >>>>> API if no tests ever touch it. >>>> Yes, definitely we need more tests with the new channels API to test other >>>> than just tcp connection. I could give a try for vsock and unix with the >>>> new QAPI syntax, and add some tests. >>>> >>>> I also wanted to bring in attention that, this solution I what i feel is >>>> also >>>> not complete. If we are using new channel syntax for migrate_qmp, then the >>>> same syntax should also be used for migrate_qmp_incoming. But we haven't >>>> touch that, and it still prints the old syntax. We might need a lot changes >>>> in design maybe to incorporate that too in new tests totally with the new >>>> syntax. >>> Adding migrate_qmp_incoming support should be relatively simple. You had >>> patches for that in another version, no? >> No Fabiano, what I meant was, in migration-test.c, change in >> migrate_incoming_qmp >> would need to change the callback function and ultimately change all the >> callback >> handlers ? In that sense, it would require a big change ? >> Inside the migrate_incoming_qmp function, adding implementation for >> channels is >> same as other migrate_* function. > You could add the parameter to migrate_incoming_qmp and use NULL when > calling. The callbacks don't need to be changed. When we add more tests > then we'd alter the callbacks accordingly. > > I might convert the file tests soon, you can leave that part to me if > you want. Okay, sure. Will leave those changes to you. >>>> Another thing that you also noted down while discussing on the patches that >>>> we should have a standard pattern on how to define the migration tests. Even >>>> that would be helpful for the users, on how to add new tests, where to add >>>> new tests in the file, and which test is needed to run if a specific change >>>> needs to be tested. >>>> >>>>>> >>>>>> iv = qobject_input_visitor_new(object); >>>>>> visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort); >>>>>> + addr = addrs->value; >>>>>> visit_free(iv); >>>>>> >>>>>> - /* we are only using a single address */ >>>>>> >>>>>> Regards, Het Gala
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c index 3e8c19c4de..8806dc841e 100644 --- a/tests/qtest/migration-helpers.c +++ b/tests/qtest/migration-helpers.c @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr) } } -static char * -migrate_get_socket_address(QTestState *who, const char *parameter) +static SocketAddress *migrate_get_socket_address(QTestState *who) { QDict *rsp; - char *result; SocketAddressList *addrs; + SocketAddress *addr; Visitor *iv = NULL; QObject *object; rsp = migrate_query(who); - object = qdict_get(rsp, parameter); + object = qdict_get(rsp, "socket-address"); iv = qobject_input_visitor_new(object); visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort); + addr = addrs->value; visit_free(iv); - /* we are only using a single address */ - result = SocketAddress_to_str(addrs->value); - - qapi_free_SocketAddressList(addrs); qobject_unref(rsp); - return result; + return addr; +} + +static char * +migrate_get_connect_uri(QTestState *who) +{ + SocketAddress *addrs; + char *connect_uri; + + addrs = migrate_get_socket_address(who); + connect_uri = SocketAddress_to_str(addrs); + + qapi_free_SocketAddress(addrs); + return connect_uri; } bool migrate_watch_for_events(QTestState *who, const char *name, @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri, g_assert(!qdict_haskey(args, "uri")); if (!uri) { - connect_uri = migrate_get_socket_address(to, "socket-address"); + connect_uri = migrate_get_connect_uri(to); } qdict_put_str(args, "uri", uri ? uri : connect_uri);