>>> On 9/2/2014 at 05:16 PM, in message <20140902091655.GB21282@xxxxxxxxxx>, "Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote: > On Tue, Sep 02, 2014 at 03:08:51AM -0600, Chun Yan Liu wrote: > > > > > > >>> On 9/2/2014 at 04:54 PM, in message <20140902085434.GA21282@xxxxxxxxxx>, > > "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 <cyliu@xxxxxxxx> > > > > --- > > > > 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. > > Whether or not it causes problems today is only half the story. I'm more > concerned about the long term problems. The use of threads is increasing > in QEMU over time, so manipulating umask() is a time-bomb waiting to strike > at some point in the future when people have forgotten that this proposed > feature exists. IMHO umask() should simply never be used when multiple > threads are running, to avoid this long term risk entirely. I agree. Now suppose when it explicitly sets the mode S_IWGRP while creating new file, group user writing permission is really expected, I think setting umask to 002 to the whole qemu process should be OK. > > 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 > :| > > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list