From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39643) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SziW9-0000FK-3b for qemu-devel@nongnu.org; Fri, 10 Aug 2012 02:16:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SziW7-0006GL-Gr for qemu-devel@nongnu.org; Fri, 10 Aug 2012 02:16:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11189) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SziW7-0006Fk-80 for qemu-devel@nongnu.org; Fri, 10 Aug 2012 02:16:15 -0400 Message-ID: <5024A725.50107@redhat.com> Date: Fri, 10 Aug 2012 00:16:05 -0600 From: Eric Blake MIME-Version: 1.0 References: <1344564649-6272-1-git-send-email-coreyb@linux.vnet.ibm.com> <1344564649-6272-8-git-send-email-coreyb@linux.vnet.ibm.com> In-Reply-To: <1344564649-6272-8-git-send-email-coreyb@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig7F3E85EE6DA891D2D3D2CD34" Subject: Re: [Qemu-devel] [PATCH v8 7/7] 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, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig7F3E85EE6DA891D2D3D2CD34 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 08/09/2012 08:10 PM, 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 s/desriptors/descriptors/ > 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 > +int monitor_fdset_get_fd(int64_t fdset_id, int flags) > +{ > + MonFdset *mon_fdset; > + MonFdsetFd *mon_fdset_fd; > + int mon_fd_flags; > + > + 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; > + } Is this right? According to the commit message, the whole point of leaving an fd in the set is to allow the fd to be reused as a dup() target for as long as the fdset is alive, even if the monitor no longer cares about the existence of the fd. But this will always skip over an fd marked for removal. Maybe this function needs a flag to say whether this is an initial open driven by an explicit user string (in which case, honor the removed flag - if the user removed the O_RDWR fd and then tries a drive_add with the same fdset, the drive_add should fail because from the user's perspective, there is no O_RDWR fd in the set); vs. an internal usage due to a reopen (use an fd even if removed is true, because we may be toggling between O_RDWR and O_RDONLY multiple times long after the monitor has already removed the fdset, based on actions that were not drive by an explicit /dev/fdset name.) > + > + mon_fd_flags =3D fcntl(mon_fdset_fd->fd, F_GETFL); > + if (mon_fd_flags =3D=3D -1) { > + return -1; > + } > + > + if ((flags & O_ACCMODE) =3D=3D (mon_fd_flags & O_ACCMODE))= { > + return mon_fdset_fd->fd; > + } I still wonder if a request for O_RDONLY should be satisfied by an existing O_RDWR fd, especially in light of the fact that libvirt would rather pass in only one RDWR fd but qemu block opening currently opens twice during probing. But if it turns out to be a problem in practice, and if libvirt can't really manage to pass two fds into the set, we can hack that in later. Meanwhile, I'm okay with this first round patch requiring an exact match. > @@ -87,6 +151,39 @@ int qemu_open(const char *name, int flags, ...) > int ret; > int mode =3D 0; > =20 > +#ifndef _WIN32 > + const char *fdset_id_str; > + > + /* Attempt dup of fd from fd set */ > + if (strstart(name, "/dev/fdset/", &fdset_id_str)) { > + int64_t fdset_id; > + int fd, dupfd; > + > + fdset_id =3D qemu_parse_fdset(fdset_id_str); > + if (fdset_id =3D=3D -1) { > + errno =3D EINVAL; > + return -1; > + } > + > + fd =3D monitor_fdset_get_fd(fdset_id, flags); > + if (fd =3D=3D -1) { > + return -1; > + } > + > + dupfd =3D qemu_dup(fd, flags); > + if (fd =3D=3D -1) { > + return -1; > + } > + > + ret =3D monitor_fdset_dup_fd_add(fdset_id, dupfd); > + if (ret =3D=3D -1) { > + return -1; Leaks dupfd (admittedly only on a corner-case failure, but still worth addressing). --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig7F3E85EE6DA891D2D3D2CD34 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/ iQEcBAEBCAAGBQJQJKclAAoJEKeha0olJ0NqGDAH/RYiRI+LHhJlsrRce5Y9oyke 6UfX2nsfHeeIO4fPRPsYZaZtaqbs0MQYMTesRwI3S//y8UMCDFOPLFe9QcEnL/9/ XWILCtGGS9EQE9S/mLh2v3r/CQ85EoWcSIy2QqgJdV52X+RopCjlfCPAESZ4G1L1 aMKI9L7mzkgrUArdf+85bXBgQh6kjgf5JcY/J/AxsPe8m2b7ZbIRk0qnCve4HPye TVuG4Hr9iTqq3vselN06z9KsGOLXOs3dKFSFZfojCW/3717USCRcXHFvsyIWWlLY ZjFFTxDDCslHwyy+6CABq17ll4ZxqTprUM96osUztVgrLSGw5PmfnBHDSN2SISs= =1X6j -----END PGP SIGNATURE----- --------------enig7F3E85EE6DA891D2D3D2CD34--