From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55457) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwG4M-0004BW-3m for qemu-devel@nongnu.org; Tue, 31 Jul 2012 13:17:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwG4K-0004Sz-IH for qemu-devel@nongnu.org; Tue, 31 Jul 2012 13:17:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6225) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwG4K-0004Sv-9H for qemu-devel@nongnu.org; Tue, 31 Jul 2012 13:17:16 -0400 Message-ID: <50181314.1040909@redhat.com> Date: Tue, 31 Jul 2012 11:17:08 -0600 From: Eric Blake MIME-Version: 1.0 References: <20120730213409.21536.7589.sendpatchset@skannery.in.ibm.com> <20120730213437.21536.87232.sendpatchset@skannery.in.ibm.com> In-Reply-To: <20120730213437.21536.87232.sendpatchset@skannery.in.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig31860D416C6FFA77F42111EA" Subject: Re: [Qemu-devel] [v2 Patch 2/9]block: raw-posix image file reopen List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Supriya Kannery Cc: Kevin Wolf , Shrinidhi Joshi , Stefan Hajnoczi , Jeff Cody , qemu-devel@nongnu.org, Luiz Capitulino , Christoph Hellwig This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig31860D416C6FFA77F42111EA Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/30/2012 03:34 PM, Supriya Kannery wrote: > raw-posix driver changes for bdrv_reopen_xx functions to > safely reopen image files. Reopening of image files while=20 > changing hostcache dynamically is handled here. >=20 > Signed-off-by: Supriya Kannery >=20 > --- > Index: qemu/block/raw.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **= prs, > + int flags) > +{ > + BDRVRawReopenState *raw_rs =3D g_malloc0(sizeof(BDRVRawReopenState= )); > + BDRVRawState *s =3D bs->opaque; > + int ret =3D 0; > + > + raw_rs->reopen_state.bs =3D bs; > + > + /* stash state before reopen */ > + raw_rs->stash_s =3D g_malloc0(sizeof(BDRVRawState)); > + raw_stash_state(raw_rs->stash_s, s); > + s->fd =3D dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC); You called it out in your cover letter, but just pointing out that this is one of the locations that needs a conditional fallback to dup2/qemu_set_cloexec if dup3 and O_CLOEXEC are missing. More importantly, though, you want this to be fcntl(F_DUP_CLOEXEC) and NOT dup3, so that you are duplicating to the first available fd rather than accidentally overwriting whatever s->fd happened to be on entry. Also, where is your error checking that you didn't just assign s->fd to -1? It looks like your goal here is to stash a copy of the fd, so that on failure you can then pivot over to your copy. > + > + *prs =3D &(raw_rs->reopen_state); > + > + /* Flags that can be set using fcntl */ > + int fcntl_flags =3D BDRV_O_NOCACHE; This variable name is misleading; fcntl_flags in my mind implies O_* not BDRV_O_*, as we are not guaranteed that they are the same values. Maybe bdrv_flags is a better name. Or are you trying to list the subset of BDRV_O flags that can be changed via mapping to the appropriate O_ flag during fcntl? > + > + if ((bs->open_flags & ~fcntl_flags) =3D=3D (flags & ~fcntl_flags))= { > + if ((flags & BDRV_O_NOCACHE)) { > + s->open_flags |=3D O_DIRECT; > + } else { > + s->open_flags &=3D ~O_DIRECT; > + } > + ret =3D fcntl_setfl(s->fd, s->open_flags); > + } else { > + > + /* close and reopen using new flags */ > + bs->drv->bdrv_close(bs); > + ret =3D bs->drv->bdrv_file_open(bs, bs->filename, flags); > + } At any rate, in spite of the poor choice of naming for fcntl_flags, the logic here looked correct - if the only BDRV_O_ bits that changed between existing flags and the requested flags can be handled by fcntl, then use fcntl to change them, otherwise open from scratch. > +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs= ) > +{ > + BDRVRawReopenState *raw_rs; > + BDRVRawState *s =3D bs->opaque; > + > + raw_rs =3D container_of(rs, BDRVRawReopenState, reopen_state); > + > + /* revert to stashed state */ > + if (s->fd !=3D -1) { > + close(s->fd); > + } At first, I worried that you needed to use dup3() here to restore s->fd from the cached fd... > + raw_revert_state(s, raw_rs->stash_s); then I read the code for raw_revert_state, where you are rewriting the state to permanently use the dup'd copy rather than trying to revert back to the old fd number. As long as we are sure that all other points in the code base always go through s->fd rather than caching the old value, and therefore don't have a stale reference, then swapping the struct instead of using dup3 to restore the exact fd value we previously had should work. --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig31860D416C6FFA77F42111EA 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/ iQEcBAEBCAAGBQJQGBMVAAoJEKeha0olJ0NqdCoH/RrJIwzPKqr1ig+CMcFM3DQa BNZrZsO5wDv5G2TMLn9zmj8XjW+WDk3gzFLvw69aqXg9AMf8yE6OmEPFlaTeBdjN ag55f2OQ72B77PaSuvccIWzu7WXx/tX1RixVsIUxpFUIr5Gl/T3yRVlRN9ghTnrS uGqZ3RgYpz+aPrvK9jm1S1lFion2pWJEnVakl+qZuxR2Ipn/EDTvUnS0dozSRFMw fehBnw7agArw1XeLP9dZieaEwVX78EO1ejG3h007KUiKrJ1Q/KkJgsmCJK6ZnJXm UUbbxOczriqvyccGYrh10FhJYwS3uHnglHWV8t7UwcqGogR4H4Ne7HEMs+k1trw= =24jT -----END PGP SIGNATURE----- --------------enig31860D416C6FFA77F42111EA--