Patchwork [13/20] oslib: add a wrapper for mmap/munmap

login
register
mail settings
Submitter Paolo Bonzini
Date Dec. 12, 2012, 1:46 p.m.
Message ID <1355319999-30627-14-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/205523/
State New
Headers show

Comments

Paolo Bonzini - Dec. 12, 2012, 1:46 p.m.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 osdep.h       | 10 ++++++++++
 oslib-posix.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 oslib-win32.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)
Eric Blake - Dec. 14, 2012, 10:54 p.m.
On 12/12/2012 06:46 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  osdep.h       | 10 ++++++++++
>  oslib-posix.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  oslib-win32.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 116 insertions(+)
> 

> +int qemu_mmap_alloc(QEMUMmapArea *mm, const char *path, size_t size)
> +{
> +    int fd = -1;
> +    char *mem = NULL;
> +    int save_errno;
> +
> +    fd = 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 = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +    if (!mem) {
> +        goto fail;
> +    }
> +
> +    mm->fd = fd;
> +    mm->mem = mem;
> +    mm->size = size;
> +    return 0;
> +
> +fail:
> +    save_errno = errno;
> +    if (fd >= 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 = 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.
Paolo Bonzini - Dec. 15, 2012, 9:06 a.m.
----- Messaggio originale -----
> Da: "Eric Blake" <eblake@redhat.com>
> A: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, kwolf@redhat.com, stefanha@redhat.com, jcody@redhat.com
> Inviato: Venerdì, 14 dicembre 2012 23:54:31
> Oggetto: Re: [PATCH 13/20] oslib: add a wrapper for mmap/munmap
> 
> On 12/12/2012 06:46 AM, Paolo Bonzini wrote:
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  osdep.h       | 10 ++++++++++
> >  oslib-posix.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  oslib-win32.c | 59
> >  +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 116 insertions(+)
> > 
> 
> > +int qemu_mmap_alloc(QEMUMmapArea *mm, const char *path, size_t
> > size)
> > +{
> > +    int fd = -1;
> > +    char *mem = NULL;
> > +    int save_errno;
> > +
> > +    fd = qemu_open(path, O_RDWR | O_CREAT, 0666);
> 
> Do you want O_EXCL and/or O_TRUNC here as well?
> 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).

Yes.  The caller will copy the transient bitmap to the persistent one
if it is creating a new file, and vice versa when loading.

> You're blindly unlinking here

Right, the unlink should be in the caller.

> ; 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?

No error checking because the mmap error is more important to pass up.

> You unlink()d the file on failure during the initial map, but nowhere
> else.  Is that intentional?

Yes.

Paolo

Patch

diff --git a/osdep.h b/osdep.h
index 87d3b9c..fb7f72c 100644
--- a/osdep.h
+++ b/osdep.h
@@ -134,6 +134,16 @@  void qemu_vfree(void *ptr);
 
 #endif
 
+typedef struct QEMUMmapArea {
+    int fd;
+    void *mem;
+    size_t size;
+} QEMUMmapArea;
+
+int qemu_mmap_alloc(QEMUMmapArea *mm, const char *path, size_t size);
+int qemu_mmap_flush(QEMUMmapArea *mm);
+void qemu_mmap_free(QEMUMmapArea *mm);
+
 int qemu_madvise(void *addr, size_t len, int advice);
 
 int qemu_open(const char *name, int flags, ...);
diff --git a/oslib-posix.c b/oslib-posix.c
index 9db9c3d..f230f61 100644
--- a/oslib-posix.c
+++ b/oslib-posix.c
@@ -61,6 +61,7 @@  static int running_on_valgrind = -1;
 #ifdef CONFIG_LINUX
 #include <sys/syscall.h>
 #endif
+#include <sys/mman.h>
 
 int qemu_get_thread_id(void)
 {
@@ -226,3 +227,49 @@  int qemu_utimens(const char *path, const struct timespec *times)
 
     return utimes(path, &tv[0]);
 }
+
+int qemu_mmap_alloc(QEMUMmapArea *mm, const char *path, size_t size)
+{
+    int fd = -1;
+    char *mem = NULL;
+    int save_errno;
+
+    fd = qemu_open(path, O_RDWR | O_CREAT, 0666);
+    if (fd < 0) {
+        goto fail;
+    }
+
+    if (ftruncate(fd, size) < 0) {
+        goto fail;
+    }
+
+    mem = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+    if (!mem) {
+        goto fail;
+    }
+
+    mm->fd = fd;
+    mm->mem = mem;
+    mm->size = size;
+    return 0;
+
+fail:
+    save_errno = errno;
+    if (fd >= 0) {
+        close(fd);
+        unlink(path);
+    }
+    return -save_errno;
+}
+
+int qemu_mmap_flush(QEMUMmapArea *mm)
+{
+    int rc = 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);
+}
diff --git a/oslib-win32.c b/oslib-win32.c
index 51b33e8..3885a31 100644
--- a/oslib-win32.c
+++ b/oslib-win32.c
@@ -150,3 +150,62 @@  int qemu_get_thread_id(void)
 {
     return GetCurrentThreadId();
 }
+
+int qemu_mmap_alloc(QEMUMmapArea *mm, const char *path, size_t size)
+{
+    int fd = -1;
+    char *mem = NULL;
+
+    HANDLE hFile, hMapping;
+    int save_errno;
+
+    fd = qemu_open(path, O_RDWR | O_CREAT, 0666);
+    if (fd < 0) {
+        goto fail;
+    }
+
+    hFile = (HANDLE)_get_osfhandle(fd);
+    if (ftruncate(fd, size) < 0) {
+        goto fail;
+    }
+
+    hMapping = CreateFileMapping(hFile, NULL, PAGE_EXECUTE_READWRITE,
+                                 0, 0, NULL);
+    if (!hMapping) {
+        goto fail;
+    }
+
+    mem = MapViewOfFileEx(hMapping, FILE_MAP_WRITE, 0, 0, size, NULL);
+    CloseHandle(hMapping);
+    if (mem) {
+        goto fail;
+    }
+
+    mm->fd = fd;
+    mm->mem = mem;
+    mm->size = size;
+    return 0;
+
+fail:
+    save_errno = errno;
+    if (fd >= 0) {
+        close(fd);
+        unlink(path);
+    }
+    return -save_errno;
+}
+
+int qemu_mmap_flush(QEMUMmapArea *mm)
+{
+    int rc;
+
+    FlushViewOfFile(mm->mem, mm->size);
+    rc = qemu_fdatasync(mm->fd);
+    return rc < 0 ? -errno : 0;
+}
+
+void qemu_mmap_free(QEMUMmapArea *mm)
+{
+    UnmapViewOfFile(mm->mem);
+    close(mm->fd);
+}