Re: [Qemu-devel] [PATCH 1/2] add 'umask' option to -chardev

[Chun Yan Liu]

>>> On 9/2/2014 at 04:54 PM, in message <address@hidden>,
"Daniel P. Berrange" <address@hidden> wrote: 
> On Tue, Sep 02, 2014 at 03:40:42PM +0800, Chunyan Liu wrote: 
> > To use virtio-serial device, unix socket created for chardev with 
> > default umask(022) has insufficient permissions. 
> >  
> > e.g. start kvm guest with: 
> > -device virtio-serial \ 
> > -chardev socket,path=/tmp/foo,server,nowait,id=foo \ 
> > -device virtserialport,chardev=foo,name=org.fedoraproject.port.0 
> >  
> > Check permissions for the socket file that has been created in the host 
> > to enable communication through virtual serial ports in the guest: 
> > #ls -l /tmp/somefile.sock 
> > srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock 
> >  
> > Other users in the qemu group (like real user, test engines, etc) cannot 
> > write to this socket. 
> >  
> > Problem reported here: 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11 
> > https://bugzilla.novell.com/show_bug.cgi?id=888166 
> >  
> > This patch tries to add a 'umask' option to 'chardev', so that user 
> > can have chance to indicate a umask overwritting the default one (default 
> > is 022), then create unix sockets with expected permissions. 
> >  
> > Signed-off-by: Chunyan Liu <address@hidden> 
> > --- 
> > This is patch for qemu. 
> >  
> >  qemu-char.c         |  3 +++ 
> >  qemu-options.hx     |  9 +++++++-- 
> >  util/qemu-sockets.c | 12 +++++++++++- 
> >  3 files changed, 21 insertions(+), 3 deletions(-) 
>  
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c 
> > index 5d38395..facf2c6 100644 
> > --- a/util/qemu-sockets.c 
> > +++ b/util/qemu-sockets.c 
> > @@ -680,7 +680,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp) 
> >  { 
> >      struct sockaddr_un un; 
> >      const char *path = qemu_opt_get(opts, "path"); 
> > -    int sock, fd; 
> > +    int newmask = qemu_opt_get_number(opts, "umask", 0); 
> > +    int sock, fd, oldmask; 
> >   
> >      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); 
> >      if (sock < 0) { 
> > @@ -708,10 +709,19 @@ int unix_listen_opts(QemuOpts *opts, Error **errp) 
> >      } 
> >   
> >      unlink(un.sun_path); 
> > +    if (newmask) { 
> > +        oldmask = umask(newmask); 
> > +    } 
> >      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) { 
> > +        if (newmask) { 
> > +            umask(oldmask); 
> > +        } 
> >          error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED); 
> >          goto err; 
> >      } 
> > +    if (newmask) { 
> > +        umask(oldmask); 
> > +    } 
>  
> Setting  umask() is not thread-safe as it affects the entire process. While 
> this is OK for chardevs which are cold-plugged at startup, once QEMU is 
> running it is not OK to alter umask during hotplug of devices. 
>  
> Wouldn't it be simpler for libvirt to simply set the umask to 002 when it 
> first launches QEMU, avoiding the need for trying todo this per device. 

I think that's OK. Only one thing: I'm not sure if permissions of any other
file created in qemu will be changed due to this change, and if that is
unexpected or not.
 
>  
> Regards, 
> Daniel 
> --  
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/  
> :| 
> |: http://libvirt.org              -o-             http://virt-manager.org  
> :| 
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/  
> :| 
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc  
> :| 
>  
>  
>