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

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 osdep.h       | 10 ++++++++++
 oslib-posix.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 oslib-win32.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)

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.

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