From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37170) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tje9O-0002xv-Ip for qemu-devel@nongnu.org; Fri, 14 Dec 2012 17:54:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tje9K-0001te-CS for qemu-devel@nongnu.org; Fri, 14 Dec 2012 17:54:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35648) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tje9K-0001tW-5B for qemu-devel@nongnu.org; Fri, 14 Dec 2012 17:54:34 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qBEMsWlQ012772 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 14 Dec 2012 17:54:33 -0500 Message-ID: <50CBAE27.1050102@redhat.com> Date: Fri, 14 Dec 2012 15:54:31 -0700 From: Eric Blake MIME-Version: 1.0 References: <1355319999-30627-1-git-send-email-pbonzini@redhat.com> <1355319999-30627-14-git-send-email-pbonzini@redhat.com> In-Reply-To: <1355319999-30627-14-git-send-email-pbonzini@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enigA59E28A75B53A0E37E74A5B7" Subject: Re: [Qemu-devel] [PATCH 13/20] oslib: add a wrapper for mmap/munmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigA59E28A75B53A0E37E74A5B7 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 12/12/2012 06:46 AM, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini > --- > osdep.h | 10 ++++++++++ > oslib-posix.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > oslib-win32.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++= ++++++++ > 3 files changed, 116 insertions(+) >=20 > +int qemu_mmap_alloc(QEMUMmapArea *mm, const char *path, size_t size) > +{ > + int fd =3D -1; > + char *mem =3D NULL; > + int save_errno; > + > + fd =3D qemu_open(path, O_RDWR | O_CREAT, 0666); Do you want O_EXCL and/or O_TRUNC here as well? > + if (fd < 0) { > + goto fail; > + } > + > + if (ftruncate(fd, size) < 0) { Or are you okay with using this to read existing contents and for the size to possibly discard the tail of a file? (Hmm, thinking of how you plan on using persistent bitmaps, it sounds like you WANT to open pre-existing files; but then the caller has to be careful to pass in the right size). Would it be any better to alter this wrapper function to allow the user to choose between creating a new file (size is relevant, and use O_EXCL) vs. re-reading an existing file (no ftruncate performed, and the mmap size is picked up from fstat())? > + goto fail; > + } > + > + mem =3D mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);= > + if (!mem) { > + goto fail; > + } > + > + mm->fd =3D fd; > + mm->mem =3D mem; > + mm->size =3D size; > + return 0; > + > +fail: > + save_errno =3D errno; > + if (fd >=3D 0) { > + close(fd); > + unlink(path); You're blindly unlinking here; sounds like you either want O_EXCL on the open, or need to skip the unlink() if the file was pre-existing. No error checking that unlink() succeeded? > + } > + return -save_errno; > +} > + > +int qemu_mmap_flush(QEMUMmapArea *mm) > +{ > + int rc =3D msync(mm->mem, mm->size, MS_SYNC); > + return rc < 0 ? -errno : 0; > +} > + > +void qemu_mmap_free(QEMUMmapArea *mm) > +{ > + munmap(mm->mem, mm->size); > + close(mm->fd); > +} You unlink()d the file on failure during the initial map, but nowhere else. Is that intentional? > diff --git a/oslib-win32.c b/oslib-win32.c My review gets weaker here... However, it looks like you have a matching implementation based on the wrapper interface you defined. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enigA59E28A75B53A0E37E74A5B7 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 undefined - http://www.enigmail.net/ iQEcBAEBCAAGBQJQy64nAAoJEKeha0olJ0NqupAIAJwGpjFEwrCGi1MlDaAT44Xr rdi25hseZIVqxTQsnl8gKZ8GLn6D74/oUDk4VpdvvyhOfM6bh8WZLQwA003pFGuk xlURA23bPMyBitLEBgrLBuqhBeqHTi1xIistYv4Rrj1ktfLqo2ihj303x8EtO9On tIzdUpUoT3OOaUQNOQuqrS/BBnoXzeH6JP/uYVJB7BVuHOCjGGDV/o33CDGXTJVD RoDHx0V6jeuANYIXbshMDKFV8Ea6L6f7jCkbxs0R/HvCPu6056duUc83apiNi8K3 ODfBIrUgT30rJ+l0EZMeKaORlIR1RjNxYauZ4oSURGI/QQHLRzpNOQ/Kq6Wxj4Y= =rwh7 -----END PGP SIGNATURE----- --------------enigA59E28A75B53A0E37E74A5B7--