Re: [Qemu-block] [PATCH 1/3] qcow2: mark the memory as no longer needed after qcow2_cache_empty()

[Alberto Garcia]

On Tue 02 Jun 2015 12:05:59 PM CEST, Kevin Wolf <address@hidden> wrote:

>> +#include <sys/mman.h>
>>  #include "block/block_int.h"
>>  #include "qemu-common.h"
>> +#include "qemu/osdep.h"
>>  #include "qcow2.h"
>>  #include "trace.h"
>
> This breaks the mingw build:
>
> /mnt/qemu/block/qcow2-cache.c:25:22: fatal error: sys/mman.h: No such file or 
> directory
>  #include <sys/mman.h>

Ok, I can use #if defined(CONFIG_MADVISE) || defined(CONFIG_POSIX_MADVISE)
as it's done in util/osdep.c for the same header.

>> +#if QEMU_MADV_DONTNEED != QEMU_MADV_INVALID
>> +    BDRVQcowState *s = bs->opaque;
>> +    void *t = qcow2_cache_get_table_addr(bs, c, i);
>> +    long align = sysconf(_SC_PAGESIZE);
>
> It seems that getpagesize() is usually used in qemu.

The getpagesize() manual page actually recommends using sysconf() for
portability reasons. But other than that I don't have a problem with
getpagesize() if it's the preferred choice in QEMU.

>> +    size_t mem_size = (size_t) s->cluster_size * num_tables;
>> +    size_t offset = QEMU_ALIGN_UP((uintptr_t) t, align) - (uintptr_t) t;
>> +    size_t length = QEMU_ALIGN_DOWN(mem_size - offset, align);
>
> Instead of all the aligning here, shouldn't we just make sure that the
> tables are created with the right alignment?

The tables should have the right alignment and their size should be a
multiple of the page size. The latter condition is not necessarily true
(a cluster can be smaller than one page) so I'd keep that code in any
case.

Berto