Re: [Qemu-devel] [PATCH 13/20] oslib: add a wrapper for mmap/munmap

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2524 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]