On 20/03/24 12:33 am, Fabiano Rosas wrote:
Het Gala<het.g...@nutanix.com> writes:
On 18/03/24 7:46 pm, Fabiano Rosas wrote:
Het Gala<het.g...@nutanix.com> writes:
On 15/03/24 6:28 pm, Fabiano Rosas wrote:
Het Gala<het.g...@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
