From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53222) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Su7UE-0005mV-QH for qemu-devel@nongnu.org; Wed, 25 Jul 2012 15:43:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Su7UD-00012s-96 for qemu-devel@nongnu.org; Wed, 25 Jul 2012 15:43:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34614) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Su7UD-00012m-0V for qemu-devel@nongnu.org; Wed, 25 Jul 2012 15:43:09 -0400 Message-ID: <50104C44.1050206@redhat.com> Date: Wed, 25 Jul 2012 13:43:00 -0600 From: Eric Blake MIME-Version: 1.0 References: <1343048885-1701-1-git-send-email-coreyb@linux.vnet.ibm.com> <1343048885-1701-7-git-send-email-coreyb@linux.vnet.ibm.com> In-Reply-To: <1343048885-1701-7-git-send-email-coreyb@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig872FD2997FE05010EC66E2DA" Subject: Re: [Qemu-devel] [PATCH v5 6/6] block: Enable qemu_open/close to work with fd sets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Bryant Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig872FD2997FE05010EC66E2DA Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/23/2012 07:08 AM, Corey Bryant wrote: > When qemu_open is passed a filename of the "/dev/fdset/nnn" > format (where nnn is the fdset ID), an fd with matching access > mode flags will be searched for within the specified monitor > fd set. If the fd is found, a dup of the fd will be returned > from qemu_open. >=20 > Each fd set has a reference count. The purpose of the reference > count is to determine if an fd set contains file descriptors that > have open dup() references that have not yet been closed. It is > incremented on qemu_open and decremented on qemu_close. It is > not until the refcount is zero that file desriptors in an fd set > can be closed. If an fd set has dup() references open, then we > must keep the other fds in the fd set open in case a reopen > of the file occurs that requires an fd with a different access > mode. >=20 > +++ b/monitor.c > @@ -2551,6 +2551,91 @@ static void monitor_fdsets_set_in_use(Monitor *m= on, bool in_use) > } > } > =20 > +void monitor_fdset_increment_refcount(Monitor *mon, int64_t fdset_id) > +{ > + mon_fdset_t *mon_fdset; > + > + if (!mon) { > + return; > + } Am I reading this code right by stating that 'if there is no monitor, we don't increment the refcount'? How does a monitor reattach affect things? Or am I missing something fundamental about the cases when 'mon=3D=3DNULL' will exist? > +int monitor_fdset_get_fd(Monitor *mon, int64_t fdset_id, int flags) > +{ > + mon_fdset_t *mon_fdset; > + mon_fdset_fd_t *mon_fdset_fd; > + int mon_fd_flags; > + > + if (!mon) { > + errno =3D ENOENT; > + return -1; > + } > + > + QLIST_FOREACH(mon_fdset, &mon->fdsets, next) { > + if (mon_fdset->id !=3D fdset_id) { > + continue; > + } > + QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) { > + if (mon_fdset_fd->removed) { > + continue; > + } > + > + mon_fd_flags =3D fcntl(mon_fdset_fd->fd, F_GETFL); > + if (mon_fd_flags =3D=3D -1) { > + return -1; This says we fail on the first fcntl() failure, instead of trying other fds in the set. Granted, an fcntl() failure is probably the sign of a bigger bug (such as closing an fd at the wrong point in time), so I guess trying to go on doesn't make much sense once we already know we are hosed. > + } > + > + switch (flags & O_ACCMODE) { > + case O_RDWR: > + if ((mon_fd_flags & O_ACCMODE) =3D=3D O_RDWR) { > + return mon_fdset_fd->fd; > + } > + break; > + case O_RDONLY: > + if ((mon_fd_flags & O_ACCMODE) =3D=3D O_RDONLY) { > + return mon_fdset_fd->fd; > + } > + break; Do we want to allow the case where the caller asked for O_RDONLY, but the set only has O_RDWR? After all, the caller is getting a compatible subset of what the set offers. > + case O_WRONLY: > + if ((mon_fd_flags & O_ACCMODE) =3D=3D O_WRONLY) { > + return mon_fdset_fd->fd; > + } > + break; Likewise, should we allow a caller asking for O_WRONLY when the set provides only O_RDWR? > =20 > +/* > + * Dups an fd and sets the flags > + */ > +static int qemu_dup(int fd, int flags) > +{ > + int i; > + int ret; > + int serrno; > + int dup_flags; > + int setfl_flags[] =3D { O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, > + O_NONBLOCK, 0 }; > + > + if (flags & O_CLOEXEC) { > + ret =3D fcntl(fd, F_DUPFD_CLOEXEC, 0); F_DUPFD_CLOEXEC is required by POSIX but not implemented on all modern OS yet; you probably need some #ifdef and/or configure guards. > + if (ret =3D=3D -1 && errno =3D=3D EINVAL) { > + ret =3D dup(fd); > + if (ret !=3D -1 && fcntl_setfl(ret, O_CLOEXEC) =3D=3D -1) = { You _can't_ call F_SETFL with O_CLOEXEC. O_CLOEXEC only causes open() to set FD_CLOEXEC; thereafter, including in the case of this dup, what you want to do is instead set FD_CLOEXEC via F_SETFD (aka call qemu_set_cloexec, not fcntl_setfl). > + goto fail; > + } > + } > + } else { > + ret =3D dup(fd); > + } > + > + if (ret =3D=3D -1) { > + goto fail; > + } > + > + dup_flags =3D fcntl(ret, F_GETFL); > + if (dup_flags =3D=3D -1) { > + goto fail; > + } > + > + if ((flags & O_SYNC) !=3D (dup_flags & O_SYNC)) { > + errno =3D EINVAL; > + goto fail; > + } > + > + /* Set/unset flags that we can with fcntl */ > + i =3D 0; > + while (setfl_flags[i] !=3D 0) { > + if (flags & setfl_flags[i]) { > + dup_flags =3D (dup_flags | setfl_flags[i]); > + } else { > + dup_flags =3D (dup_flags & ~setfl_flags[i]); > + } > + i++; > + } Rather than looping one bit at a time, you should do this as a mask operation. > + > + if (fcntl(ret, F_SETFL, dup_flags) =3D=3D -1) { > + goto fail; > + } > + > + /* Truncate the file in the cases that open() would truncate it */= > + if (flags & O_TRUNC || > + ((flags & (O_CREAT | O_EXCL)) =3D=3D (O_CREAT | O_EXCL))) = { > + if (ftruncate(ret, 0) =3D=3D -1) { > + goto fail; > + } > + } > + > + qemu_set_cloexec(ret); If we're going to blindly set FD_CLOEXEC at the end of the day, rather than try to honor O_CLOEXEC, then why not simplify the beginning of this function: ret =3D fcntl(fd, F_DUPFD_CLOEXEC, 0); if (ret =3D=3D -1 && errno =3D=3D EINVAL) { ret =3D dup(fd); if (ret !=3D -1) { qemu_set_cloexec(ret); } } if (ret =3D=3D -1) { goto fail; } --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig872FD2997FE05010EC66E2DA Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBCAAGBQJQEExEAAoJEKeha0olJ0Nq6h8IAJdTRJgK5iZQ+H9Ok6J19HM6 gKESEQRW5woK/yGeUx0b11cm8zgJsZkLpniEDgAdPwxdDiwMuvIKogX3kP9zU2W/ 4Otje7SpNfF0K3r1pAEJHe3+zCoAvQEGPYZ2rVWlt98ZILBZ2I3fYMjNwfOz48R+ meY89OaZEAfI4gTqpWH09RGUGQcAP3KiNmwRzfDhFcXyr3rO7A2wxoE3TF9cOreD Yaj6abMOCg7NWXKittZe7taof/Hq5fbb9R+5zur5GpIGgI+F3sg5s28gXgQdkkVb 3/onVmeUF7eo15peyzfTpIANV02vk+eZS1Cn8/lFv/NsojU5pAYsck1WIDoIk9I= =5PaY -----END PGP SIGNATURE----- --------------enig872FD2997FE05010EC66E2DA--