From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49167) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgwmJ-0004EG-FW for qemu-devel@nongnu.org; Tue, 29 Sep 2015 11:25:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZgwmG-0007NV-4l for qemu-devel@nongnu.org; Tue, 29 Sep 2015 11:25:15 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:34621) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgwmF-0007NJ-Sl for qemu-devel@nongnu.org; Tue, 29 Sep 2015 11:25:12 -0400 Date: Tue, 29 Sep 2015 11:25:04 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1449381360.19918275.1443540304770.JavaMail.zimbra@redhat.com> In-Reply-To: <20150929174638-mutt-send-email-mst@redhat.com> References: <1438864852-4939-1-git-send-email-marcandre.lureau@redhat.com> <1438864852-4939-4-git-send-email-marcandre.lureau@redhat.com> <20150929174638-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 03/16] util: add memfd helpers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: haifeng lin , thibaut collet , jasowang@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, marcandre lureau ----- Original Message ----- > On Thu, Aug 06, 2015 at 02:40:39PM +0200, marcandre.lureau@redhat.com wro= te: > > From: Marc-Andr=C3=A9 Lureau > >=20 > > Add qemu_memfd_alloc/free() helpers. > >=20 > > The function helps to allocate and seal a memfd, and implements an > > open/unlink/mmap fallback for system that do not support memfd. > >=20 > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > --- > > include/qemu/memfd.h | 4 +++ > > util/memfd.c | 74 > > ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > 2 files changed, 76 insertions(+), 2 deletions(-) > >=20 > > diff --git a/include/qemu/memfd.h b/include/qemu/memfd.h > > index 8b1fe6a..950fb88 100644 > > --- a/include/qemu/memfd.h > > +++ b/include/qemu/memfd.h > > @@ -17,4 +17,8 @@ > > #define F_SEAL_WRITE 0x0008 /* prevent writes */ > > #endif > > =20 > > +void *qemu_memfd_alloc(const char *name, size_t size, unsigned int sea= ls, > > + int *fd); > > +void qemu_memfd_free(void *ptr, size_t size, int fd); > > + > > #endif /* QEMU_MEMFD_H */ > > diff --git a/util/memfd.c b/util/memfd.c > > index a98d57e..8b2b785 100644 > > --- a/util/memfd.c > > +++ b/util/memfd.c > > @@ -27,6 +27,14 @@ > > =20 > > #include "config-host.h" > > =20 > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > + > > #include "qemu/memfd.h" > > =20 > > #ifdef CONFIG_MEMFD > > @@ -44,13 +52,75 @@ > > #define MFD_ALLOW_SEALING 0x0002U > > #endif > > =20 > > -static inline int memfd_create(const char *name, unsigned int flags) > > +static int memfd_create(const char *name, unsigned int flags) > > { > > return syscall(__NR_memfd_create, name, flags); > > } > > #else /* !LINUX */ > > -static inline int memfd_create(const char *name, unsigned int flags) > > +static int memfd_create(const char *name, unsigned int flags) > > { > > return -1; > > } > > #endif > > + > > +void *qemu_memfd_alloc(const char *name, size_t size, unsigned int sea= ls, > > + int *fd) > > +{ > > + void *ptr; > > + int mfd; > > + > > + mfd =3D memfd_create(name, MFD_ALLOW_SEALING|MFD_CLOEXEC); >=20 >=20 > Hmm. Does this interact correctly with the -mem-prealloc flag? It's unrelated imho. It's helper here. In the rest of the series, it's used at runtime when migrating with variabl= e size (today code doesn't prealloc that either) >=20 > > + if (mfd !=3D -1) { > > + if (ftruncate(mfd, size) =3D=3D -1) { >=20 > Any limitations on size? not that I know (reading memfd_create) >=20 > > + perror("ftruncate"); > > + close(mfd); > > + return NULL; > > + } > > + > > + if (fcntl(mfd, F_ADD_SEALS, seals) =3D=3D -1) { > > + perror("fcntl"); > > + close(mfd); > > + return NULL; > > + } > > + } else { > > + const char *tmpdir =3D getenv("TMPDIR"); > > + gchar *fname; > > + > > + tmpdir =3D tmpdir ? tmpdir : "/tmp"; > > + > > + fname =3D g_strdup_printf("%s/memfd-XXXXXX", tmpdir); >=20 > This means there's now work to be done to set up selinux > to allow QEMU creating memfd under /tmp. doesn't sound unreasonable to me >=20 > Maybe it's better to just fail gracefully for now. it's a fallback, but sure we can remove it and add it back later if needed > > + mfd =3D mkstemp(fname); > > + unlink(fname); > > + g_free(fname); > > + > > + if (mfd =3D=3D -1) { > > + perror("mkstemp"); > > + return NULL; > > + } > > + > > + if (ftruncate(mfd, size) =3D=3D -1) { > > + perror("ftruncate"); > > + close(mfd); > > + return NULL; > > + } > > + } > > + > > + ptr =3D mmap(0, size, PROT_READ|PROT_WRITE, MAP_SHARED, mfd, 0); >=20 > Pls add space around | here and elsewhere. >=20 ok >=20 > > + if (ptr =3D=3D MAP_FAILED) { > > + perror("mmap"); > > + close(mfd); > > + return NULL; > > + } > > + > > + *fd =3D mfd; > > + return ptr; > > +} > > + > > +void qemu_memfd_free(void *ptr, size_t size, int fd) > > +{ > > + if (ptr) { > > + munmap(ptr, size); > > + } > > + > > + close(fd); >=20 > I notice you close fd unconditionally, but it's only returned > on success above. So this will close an uninitialized one? Ok, I'll add a -1 check