Re: [PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala <het.gala@nutanix.com>]

On 17/01/23 4:13 pm, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote:
>>> From: Author Het Gala <het.gala@nutanix.com>
>>>
>>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>>> of destination interface and corresponding port number in the form
>>> of a unified string 'uri' parameter. This scheme does seem to have an issue
>>> in it, i.e. double-level encoding of URIs.
>>>
>>> The current patch maps existing QAPI design into a well-defined data
>>> structure - 'MigrateChannel' only from the design perspective. Please note that
>>> the existing 'uri' parameter is kept untouched for backward compatibility.
>>>
>>> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command
>> that is passed
>>
>>    const char *argv[] = { "/bin/sh", "-c", command, NULL };
>>
>> I have a strong preference for making it possible to avoid use
>> of shell when spawning commands, since much of thue time it is
>> not required and has the potential to open up vulnerabilities.
>> It would be nice to be able to just take the full argv directly
>> IOW
>>
>>   { 'struct': 'MigrateExecAddr',
>>      'data' : {'argv': ['str'] } }
>>
>> If the caller wants to keep life safe and simple now they can
>> use
>>     ["/bin/nc", "-U", "/some/sock"]
>>
>> but if they still want to send it via shell, they can also do
>> so
>>
>>     ["/bin/sh", "-c", "...arbitrary shell script code...."]
>>
>>> +
>>> +##
>>> +# @MigrateRdmaAddr:
>>> +#
>>> +# Since 8.0
>>> +##
>>> +{ 'struct': 'MigrateRdmaAddr',
>>> +   'data' : {'rdma-str': 'str' } }
>> Loooking at the RDMA code it takes the str, and treats it
>> as an IPv4 address:
>>
>>
>>          addr = g_new(InetSocketAddress, 1);
>>          if (!inet_parse(addr, host_port, NULL)) {
>>              rdma->port = atoi(addr->port);
>>              rdma->host = g_strdup(addr->host);
>>              rdma->host_port = g_strdup(host_port);
>>          }
>>
>> so we really ought to accept an InetSocketAddress struct
>> directly
>>
>>   { 'struct': 'MigrateRdmaAddr',
>>      'data' : {'rdma-str': 'InetSocketAddress' } }
> I think that's probably the right thing to do; there is a native RDMA
> addressing scheme that people occasionally (once a decade or so)
> ask about but I don't think we've ever supported it.
>
> Dave
Yes Dave. I will be implementing Rdma in form of InetSocketAddress only 
in the upcoming patch.
>>> +
>>> +##
>>> +# @MigrateAddress:
>>> +#
>>> +# The options available for communication transport mechanisms for migration
>>> +#
>>> +# Since 8.0
>>> +##
>>> +{ 'union' : 'MigrateAddress',
>>> +  'base' : { 'transport' : 'MigrateTransport'},
>>> +  'discriminator' : 'transport',
>>> +  'data' : {
>>> +    'socket' : 'MigrateSocketAddr',
>>> +    'exec' : 'MigrateExecAddr',
>>> +    'rdma': 'MigrateRdmaAddr' } }
>>> +
>> With regards,
>> Daniel
>> -- 
>> |: https://urldefense.proofpoint.com/v2/url?u=https-3A__berrange.com&d=DwIDAw&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=p8peRp4ioaDxoipqUtW15yQdVtCPnDBQv-1wk3r3y41SXWuI5JUUPMATMyMNDI4q&s=hukETUEPKU_01AyhkaMiQFWSRE1kUv84DdpSGvVjr1Q&e=       -o-    https://urldefense.proofpoint.com/v2/url?u=https-3A__www.flickr.com_photos_dberrange&d=DwIDAw&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=p8peRp4ioaDxoipqUtW15yQdVtCPnDBQv-
>>
Regards,
Het Gala