[Qemu-devel] [RFC Patch 1/3] chardev: add new socket fd parameter for unix socket

[Qemu-devel] [RFC Patch 1/3] chardev: add new socket fd parameter for unix socket

[Wei Xu]

There has been comments on this patch, but i forgot adding this patch to 
the list, just forward it again.

When manage VMs via libvirt, qemu ofter runs with limited permission,
thus qemu can't create a file/socket, this patch is to  add a new
parameter 'sockfd' to accept fd opened and passed in from libvirt.

Signed-off-by: Wei Xu <wexu@redhat.com>
---
  qapi-schema.json | 3 ++-
  qemu-char.c      | 3 +++
  2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 8483bdf..e9f0268 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2921,7 +2921,8 @@
  ##
  { 'struct': 'UnixSocketAddress',
    'data': {
-    'path': 'str' } }
+    'path': 'str',
+    'sockfd': 'int32' } }

  ##
  # @SocketAddress
diff --git a/qemu-char.c b/qemu-char.c
index b597ee1..ea9c02e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4116,6 +4116,9 @@ QemuOptsList qemu_chardev_opts = {
              .name = "path",
              .type = QEMU_OPT_STRING,
          },{
+            .name = "sockfd",
+            .type = QEMU_OPT_NUMBER,
+        },{
              .name = "host",
              .type = QEMU_OPT_STRING,
          },{
-- 
2.7.1

Re: [Qemu-devel] [RFC Patch 1/3] chardev: add new socket fd parameter for unix socket

[Eric Blake]

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

On 06/22/2016 09:25 AM, Wei Xu wrote:
> There has been comments on this patch, but i forgot adding this patch to
> the list, just forward it again.
> 
> When manage VMs via libvirt, qemu ofter runs with limited permission,
> thus qemu can't create a file/socket, this patch is to  add a new
> parameter 'sockfd' to accept fd opened and passed in from libvirt.
> 
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>  qapi-schema.json | 3 ++-
>  qemu-char.c      | 3 +++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8483bdf..e9f0268 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2921,7 +2921,8 @@
>  ##
>  { 'struct': 'UnixSocketAddress',
>    'data': {
> -    'path': 'str' } }
> +    'path': 'str',
> +    'sockfd': 'int32' } }

Missing documentation.

This makes the new 'sockfd' parameter mandatory, but SocketAddress is an
input type.  This is not backwards compatible.  At best, you'd want to
make it optional, but I'm not even convinced you want to add it, since
we already can use the magic /dev/fdset/nnn in 'path' to pass an
arbitrary fd if the underlying code uses qemu_open().

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC Patch 1/3] chardev: add new socket fd parameter for unix socket

[Wei Xu]

On 2016年06月22日 23:39, Eric Blake wrote:
> On 06/22/2016 09:25 AM, Wei Xu wrote:
>> There have been comments on this patch, but i forgot adding this patch to
>> the list, just forward it again.
>>
>> When manage VMs via libvirt, qemu ofter runs with limited permission,
>> thus qemu can't create a file/socket, this patch is to  add a new
>> parameter 'sockfd' to accept fd opened and passed in from libvirt.
>>
>> Signed-off-by: Wei Xu <wexu@redhat.com>
>> ---
>>   qapi-schema.json | 3 ++-
>>   qemu-char.c      | 3 +++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 8483bdf..e9f0268 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2921,7 +2921,8 @@
>>   ##
>>   { 'struct': 'UnixSocketAddress',
>>     'data': {
>> -    'path': 'str' } }
>> +    'path': 'str',
>> +    'sockfd': 'int32' } }
>
> Missing documentation.
>
> This makes the new 'sockfd' parameter mandatory, but SocketAddress is an
> input type.  This is not backwards compatible.  At best, you'd want to
> make it optional, but I'm not even convinced you want to add it, since
> we already can use the magic /dev/fdset/nnn in 'path' to pass an
> arbitrary fd if the underlying code uses qemu_open().
>
Thanks for commenting it again, i was going to forward it to the list 
and ask some questions.:)

Actually i'm going to try the magic way as you suggested, just a few 
questions about that.

Command line change should be like this according to my understanding.

Current command line:
-chardev socket,id=char0,path=/var/run/openvswitch/vhost-user1,server

New command line:
qemu-kvm -add-fd fd=3,set=2,opaque="/var/run/openvswitch/vhost-user1"
-chardev socket,id=char0,path=/dev/fdset/3,server


Q1. The 'sockfd' is not used anymore, but looks the 'path' parameter is 
still mandatory AFAIK, because it's a unix domain socket, which is 
different with a network tcp/udp socket, it works like a pipe file for 
local communication only, and the 'path' parameter is a must-have 
condition before a real connect could be established, it needs a bind() 
operation to a specific path before either connect() or listen(), this 
is caused by libvirt only takes the responsibility to creates a socket 
and pass the 'fd' in only, there is nothing more about the bind(), thus 
i think qemu will have to bind() it by itself, i'm thinking maybe 
'opaque' can be used for this case.

Q2. Do you know how can i test it? i'm going to fork a process first and 
create a socket like libvirt, then exec qemu and pass it in, just 
wondering how can i map it to '/dev/fdset/' directory after created the 
socket?

Q3.
-------------------------------------------------------------------
Daniel's comment before:
'Path' refers to a UNIX domain socket path, so the code won't be using
qemu_open() - it'll be using the sockets APIs to open/create a UNIX
socket. I don't think qemu_open() would even work with a socket FD,
since it'll be trying to set various modes on the FD via fcntl() that
don't work with sockets AFAIK
-------------------------------------------------------------------
Seems what i should call is qemu_socket() to fill in qemu_open(), i 
should check if it's started as '/dev/fdset' like in qemu_open(), and 
just pick the 'fd' up, is this enough? should i check the modes?

Thanks,
Wei

Re: [Qemu-devel] [RFC Patch 1/3] chardev: add new socket fd parameter for unix socket

[Wei Xu]

Anyone can help on this, or at least help me with question 2 about how 
to test it using 'add-fd' command line?

Q2. Does anybody know how can i test it? i'm going to fork a process 
firstly and create a socket like libvirt, then exec qemu and pass it in, 
just wondering how can i map the socket to '/dev/fdset/' directory after 
created the socket? Another issue is if 'set=2' is a right usage for a 
socket fd, any comment?

Command line:
qemu-kvm -add-fd fd=3,set=2,opaque="/var/run/openvswitch/vhost-user1"
          -chardev socket,id=char0,path=/dev/fdset/3,server


On 2016年06月23日 00:46, Wei Xu wrote:
> On 2016年06月22日 23:39, Eric Blake wrote:
>> On 06/22/2016 09:25 AM, Wei Xu wrote:
>>> There have been comments on this patch, but i forgot adding this
>>> patch to
>>> the list, just forward it again.
>>>
>>> When manage VMs via libvirt, qemu ofter runs with limited permission,
>>> thus qemu can't create a file/socket, this patch is to  add a new
>>> parameter 'sockfd' to accept fd opened and passed in from libvirt.
>>>
>>> Signed-off-by: Wei Xu <wexu@redhat.com>
>>> ---
>>>   qapi-schema.json | 3 ++-
>>>   qemu-char.c      | 3 +++
>>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index 8483bdf..e9f0268 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -2921,7 +2921,8 @@
>>>   ##
>>>   { 'struct': 'UnixSocketAddress',
>>>     'data': {
>>> -    'path': 'str' } }
>>> +    'path': 'str',
>>> +    'sockfd': 'int32' } }
>>
>> Missing documentation.
>>
>> This makes the new 'sockfd' parameter mandatory, but SocketAddress is an
>> input type.  This is not backwards compatible.  At best, you'd want to
>> make it optional, but I'm not even convinced you want to add it, since
>> we already can use the magic /dev/fdset/nnn in 'path' to pass an
>> arbitrary fd if the underlying code uses qemu_open().
>>
> Thanks for commenting it again, i was going to forward it to the list
> and ask some questions.:)
>
> Actually i'm going to try the magic way as you suggested, just a few
> questions about that.
>
> Command line change should be like this according to my understanding.
>
> Current command line:
> -chardev socket,id=char0,path=/var/run/openvswitch/vhost-user1,server
>
> New command line:
> qemu-kvm -add-fd fd=3,set=2,opaque="/var/run/openvswitch/vhost-user1"
> -chardev socket,id=char0,path=/dev/fdset/3,server
>
>
> Q1. The 'sockfd' is not used anymore, but looks the 'path' parameter is
> still mandatory AFAIK, because it's a unix domain socket, which is
> different with a network tcp/udp socket, it works like a pipe file for
> local communication only, and the 'path' parameter is a must-have
> condition before a real connect could be established, it needs a bind()
> operation to a specific path before either connect() or listen(), this
> is caused by libvirt only takes the responsibility to creates a socket
> and pass the 'fd' in only, there is nothing more about the bind(), thus
> i think qemu will have to bind() it by itself, i'm thinking maybe
> 'opaque' can be used for this case.
>
> Q2. Do you know how can i test it? i'm going to fork a process first and
> create a socket like libvirt, then exec qemu and pass it in, just
> wondering how can i map it to '/dev/fdset/' directory after created the
> socket?
>
> Q3.
> -------------------------------------------------------------------
> Daniel's comment before:
> 'Path' refers to a UNIX domain socket path, so the code won't be using
> qemu_open() - it'll be using the sockets APIs to open/create a UNIX
> socket. I don't think qemu_open() would even work with a socket FD,
> since it'll be trying to set various modes on the FD via fcntl() that
> don't work with sockets AFAIK
> -------------------------------------------------------------------
> Seems what i should call is qemu_socket() to fill in qemu_open(), i
> should check if it's started as '/dev/fdset' like in qemu_open(), and
> just pick the 'fd' up, is this enough? should i check the modes?
>
> Thanks,
> Wei

Re: [Qemu-devel] [RFC Patch 1/3] chardev: add new socket fd parameter for unix socket

[Michael S. Tsirkin]

On Thu, Jun 23, 2016 at 12:46:46AM +0800, Wei Xu wrote:
> On 2016年06月22日 23:39, Eric Blake wrote:
> > On 06/22/2016 09:25 AM, Wei Xu wrote:
> > > There have been comments on this patch, but i forgot adding this patch to
> > > the list, just forward it again.
> > > 
> > > When manage VMs via libvirt, qemu ofter runs with limited permission,
> > > thus qemu can't create a file/socket, this patch is to  add a new
> > > parameter 'sockfd' to accept fd opened and passed in from libvirt.
> > > 
> > > Signed-off-by: Wei Xu <wexu@redhat.com>
> > > ---
> > >   qapi-schema.json | 3 ++-
> > >   qemu-char.c      | 3 +++
> > >   2 files changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 8483bdf..e9f0268 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -2921,7 +2921,8 @@
> > >   ##
> > >   { 'struct': 'UnixSocketAddress',
> > >     'data': {
> > > -    'path': 'str' } }
> > > +    'path': 'str',
> > > +    'sockfd': 'int32' } }
> > 
> > Missing documentation.
> > 
> > This makes the new 'sockfd' parameter mandatory, but SocketAddress is an
> > input type.  This is not backwards compatible.  At best, you'd want to
> > make it optional, but I'm not even convinced you want to add it, since
> > we already can use the magic /dev/fdset/nnn in 'path' to pass an
> > arbitrary fd if the underlying code uses qemu_open().
> > 
> Thanks for commenting it again, i was going to forward it to the list and
> ask some questions.:)
> 
> Actually i'm going to try the magic way as you suggested, just a few
> questions about that.
> 
> Command line change should be like this according to my understanding.
> 
> Current command line:
> -chardev socket,id=char0,path=/var/run/openvswitch/vhost-user1,server
> 
> New command line:
> qemu-kvm -add-fd fd=3,set=2,opaque="/var/run/openvswitch/vhost-user1"
> -chardev socket,id=char0,path=/dev/fdset/3,server
> 
> 
> Q1. The 'sockfd' is not used anymore, but looks the 'path' parameter is
> still mandatory AFAIK, because it's a unix domain socket, which is different
> with a network tcp/udp socket, it works like a pipe file for local
> communication only, and the 'path' parameter is a must-have condition before
> a real connect could be established, it needs a bind() operation to a
> specific path before either connect() or listen(), this is caused by libvirt
> only takes the responsibility to creates a socket and pass the 'fd' in only,
> there is nothing more about the bind(), thus i think qemu will have to
> bind() it by itself, i'm thinking maybe 'opaque' can be used for this case.

I think libvirt will have to bind it.
Passing in an unbound socket wouldn't make sense.


> Q2. Do you know how can i test it? i'm going to fork a process first and
> create a socket like libvirt, then exec qemu and pass it in, just wondering
> how can i map it to '/dev/fdset/' directory after created the socket?

/dev/fdset/ is QEMU syntax to pass fd numbers where path is normally
used.

> Q3.
> -------------------------------------------------------------------
> Daniel's comment before:
> 'Path' refers to a UNIX domain socket path, so the code won't be using
> qemu_open() - it'll be using the sockets APIs to open/create a UNIX
> socket. I don't think qemu_open() would even work with a socket FD,
> since it'll be trying to set various modes on the FD via fcntl() that
> don't work with sockets AFAIK
> -------------------------------------------------------------------
> Seems what i should call is qemu_socket() to fill in qemu_open(), i should
> check if it's started as '/dev/fdset' like in qemu_open(), and just pick the
> 'fd' up, is this enough? should i check the modes?
> 
> Thanks,
> Wei

Re: [Qemu-devel] [RFC Patch 1/3] chardev: add new socket fd parameter for unix socket

[Wei Xu]

On 2016年06月28日 14:48, Michael S. Tsirkin wrote:
> On Thu, Jun 23, 2016 at 12:46:46AM +0800, Wei Xu wrote:
>> On 2016年06月22日 23:39, Eric Blake wrote:
>>> On 06/22/2016 09:25 AM, Wei Xu wrote:
>>>> There have been comments on this patch, but i forgot adding this patch to
>>>> the list, just forward it again.
>>>>
>>>> When manage VMs via libvirt, qemu ofter runs with limited permission,
>>>> thus qemu can't create a file/socket, this patch is to  add a new
>>>> parameter 'sockfd' to accept fd opened and passed in from libvirt.
>>>>
>>>> Signed-off-by: Wei Xu <wexu@redhat.com>
>>>> ---
>>>>    qapi-schema.json | 3 ++-
>>>>    qemu-char.c      | 3 +++
>>>>    2 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>> index 8483bdf..e9f0268 100644
>>>> --- a/qapi-schema.json
>>>> +++ b/qapi-schema.json
>>>> @@ -2921,7 +2921,8 @@
>>>>    ##
>>>>    { 'struct': 'UnixSocketAddress',
>>>>      'data': {
>>>> -    'path': 'str' } }
>>>> +    'path': 'str',
>>>> +    'sockfd': 'int32' } }
>>>
>>> Missing documentation.
>>>
>>> This makes the new 'sockfd' parameter mandatory, but SocketAddress is an
>>> input type.  This is not backwards compatible.  At best, you'd want to
>>> make it optional, but I'm not even convinced you want to add it, since
>>> we already can use the magic /dev/fdset/nnn in 'path' to pass an
>>> arbitrary fd if the underlying code uses qemu_open().
>>>
>> Thanks for commenting it again, i was going to forward it to the list and
>> ask some questions.:)
>>
>> Actually i'm going to try the magic way as you suggested, just a few
>> questions about that.
>>
>> Command line change should be like this according to my understanding.
>>
>> Current command line:
>> -chardev socket,id=char0,path=/var/run/openvswitch/vhost-user1,server
>>
>> New command line:
>> qemu-kvm -add-fd fd=3,set=2,opaque="/var/run/openvswitch/vhost-user1"
>> -chardev socket,id=char0,path=/dev/fdset/3,server
>>
>>
>> Q1. The 'sockfd' is not used anymore, but looks the 'path' parameter is
>> still mandatory AFAIK, because it's a unix domain socket, which is different
>> with a network tcp/udp socket, it works like a pipe file for local
>> communication only, and the 'path' parameter is a must-have condition before
>> a real connect could be established, it needs a bind() operation to a
>> specific path before either connect() or listen(), this is caused by libvirt
>> only takes the responsibility to creates a socket and pass the 'fd' in only,
>> there is nothing more about the bind(), thus i think qemu will have to
>> bind() it by itself, i'm thinking maybe 'opaque' can be used for this case.
>
> I think libvirt will have to bind it.
> Passing in an unbound socket wouldn't make sense.
Yes, but libvirt only cares about the creation for all sockets, not sure 
if this will break the rule and make libvirt considering more beyond 
it's responsiblility, actually the 'opaque' parameter in the command 
line is ok for qemu to get the path info and bind it.
>
>
>> Q2. Do you know how can i test it? i'm going to fork a process first and
>> create a socket like libvirt, then exec qemu and pass it in, just wondering
>> how can i map it to '/dev/fdset/' directory after created the socket?
>
> /dev/fdset/ is QEMU syntax to pass fd numbers where path is normally
> used.
I see, thanks.
>
>> Q3.
>> -------------------------------------------------------------------
>> Daniel's comment before:
>> 'Path' refers to a UNIX domain socket path, so the code won't be using
>> qemu_open() - it'll be using the sockets APIs to open/create a UNIX
>> socket. I don't think qemu_open() would even work with a socket FD,
>> since it'll be trying to set various modes on the FD via fcntl() that
>> don't work with sockets AFAIK
>> -------------------------------------------------------------------
>> Seems what i should call is qemu_socket() to fill in qemu_open(), i should
>> check if it's started as '/dev/fdset' like in qemu_open(), and just pick the
>> 'fd' up, is this enough? should i check the modes?
>>
>> Thanks,
>> Wei
>