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

["Daniel P. Berrange" <berrange@redhat.com>]

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 <cyliu@suse.com>
> ---
> 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.

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 :|