Re: [PATCH v2 4/6] migration: Avoid multiple parsing of uri in migration code flow

[Het Gala]

On 09/02/23 4:10 pm, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 09:35:58AM +0000, Het Gala wrote:
> > In existing senario, 'migrate' QAPI argument - string uri, is encoded
> > twice to extract migration parameters for stream connection. This is
> > not a good representation of migration wire protocol as it is a data
> > encoding scheme within a data encoding scheme. Qemu should be able to
> > directly work with results from QAPI without having to do a second
> > level parsing.
> > Modified 'migrate' QAPI design supports well defined MigrateChannel
> > struct which plays important role in avoiding double encoding
> > of uri strings.
> >
> > qemu_uri_parsing() parses uri string (kept for backward
> > compatibility) and populate the MigrateChannel struct parameters.
> > Migration code flow for all required migration transport types -
> > socket, exec and rdma is modified.
> > diff --git a/migration/migration.c b/migration/migration.c
> > index f6dd8dbb03..accbf72a18 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -63,6 +63,7 @@
> >   #include "sysemu/cpus.h"
> >   #include "yank_functions.h"
> >   #include "sysemu/qtest.h"
> > +#include "qemu/sockets.h"
> >   #include "ui/qemu-spice.h"
> >   
> >   #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
> > @@ -489,6 +490,44 @@ void migrate_add_address(SocketAddress *address)
> >                         QAPI_CLONE(SocketAddress, address));
> >   }
> >   
> > +static bool migrate_uri_parse(const char *uri,
> > +                              MigrateChannel **channel,
> > +                              Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    MigrateChannel *val = g_new0(MigrateChannel, 1);
> > +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> > +    SocketAddress *saddr = g_new0(SocketAddress, 1);
> > +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
> > +
> > +    if (strstart(uri, "exec:", NULL)) {
> > +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> > +        addrs->u.exec.data = str_split_at_comma(uri + strlen("exec:"));
> Huh, what is the purpose of using 'str_split_at_comma' ? The format
> of this is  "exec:<shell command>", because it is run as:
>
>       const char *argv[] = { "/bin/sh", "-c", command, NULL };
>
> we should not be trying to parse the bit after 'exec:' at all,
> and certainly not splitting it on commas which makes no sense
> for a shell command.
>
> I would have expected something more like this:
>
>      strList **tail = &addrs->u.exec.data;
>      QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
>      QAPI_LIST_APPEND(tail, g_strdup("-c"));
>      QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));

Oh, my bad Daniel. I thought for exec as string it would represent 
something like "exec:/bin/sh,-c,<shell-command>". But you are right.

Because I interpreted it wrongly, I wanted to include this function 
'str_split_at_comma' and that's the reason, I had to introduce first 
patch unecessarily.

Thankyou for pointing it out Daniel. I will look into it properly in the 
upcoming patchset, and your solution also makes sense.

> > +    addrs = channel->addr;
> > +    saddr = channel->addr->u.socket.socket_type;
> > +    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
> > +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> > +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> > +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> > +            migrate_protocol_allow_multi_channels(true);
> > +            socket_start_outgoing_migration(s, saddr, &local_err);
> > +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> > +            fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
> This is probably a sign that  fd_start_outgoing_migration() should
> be folded into the socket_start_outgoing_migration() method, but
> that's a separate cleanup from this patch.
I agree Daniel. 'fd' being a part of SocketAddress, here need to show it 
explicitly makes less sense.
> With regards,
> Daniel
Regards,
Het Gala