Re: [PATCH 20/41] include: move qemu_msync() to osdep

[Daniel P . Berrangé]

On Wed, Apr 20, 2022 at 05:26:03PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The implementation depends on the OS. (and longer-term goal is to move
> cutils to a common subproject)

Common with what other thing(s) ?

IMHO alot of cutils should just go away in favour of using more glib
APIs, and/or be split up, since its a random bag of largely unrelated
bits.



> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/cutils.h |  1 -
>  include/qemu/osdep.h  | 13 +++++++++++++
>  util/cutils.c         | 38 --------------------------------------
>  util/oslib-posix.c    | 18 ++++++++++++++++++
>  util/oslib-win32.c    | 10 ++++++++++
>  5 files changed, 41 insertions(+), 39 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index e873bad36674..fb47ec931876 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -130,7 +130,6 @@ const char *qemu_strchrnul(const char *s, int c);
>  #endif
>  time_t mktimegm(struct tm *tm);
>  int qemu_fdatasync(int fd);
> -int qemu_msync(void *addr, size_t length, int fd);
>  int qemu_parse_fd(const char *param);
>  int qemu_strtoi(const char *nptr, const char **endptr, int base,
>                  int *result);
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 14b6b65a5fa9..bf4f75dcde8f 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -641,6 +641,19 @@ static inline void qemu_reset_optind(void)
>  #endif
>  }
>  
> +/**
> + * Sync changes made to the memory mapped file back to the backing
> + * storage. For POSIX compliant systems this will fallback
> + * to regular msync call. Otherwise it will trigger whole file sync
> + * (including the metadata case there is no support to skip that otherwise)
> + *
> + * @addr   - start of the memory area to be synced
> + * @length - length of the are to be synced
> + * @fd     - file descriptor for the file to be synced
> + *           (mandatory only for POSIX non-compliant systems)
> + */
> +int qemu_msync(void *addr, size_t length, int fd);
> +
>  /**
>   * qemu_get_host_name:
>   * @errp: Error object
> diff --git a/util/cutils.c b/util/cutils.c
> index a01a3a754049..c0775bb53c29 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -160,44 +160,6 @@ int qemu_fdatasync(int fd)
>  #endif
>  }
>  
> -/**
> - * Sync changes made to the memory mapped file back to the backing
> - * storage. For POSIX compliant systems this will fallback
> - * to regular msync call. Otherwise it will trigger whole file sync
> - * (including the metadata case there is no support to skip that otherwise)
> - *
> - * @addr   - start of the memory area to be synced
> - * @length - length of the are to be synced
> - * @fd     - file descriptor for the file to be synced
> - *           (mandatory only for POSIX non-compliant systems)
> - */
> -int qemu_msync(void *addr, size_t length, int fd)
> -{
> -#ifdef CONFIG_POSIX
> -    size_t align_mask = ~(qemu_real_host_page_size() - 1);
> -
> -    /**
> -     * There are no strict reqs as per the length of mapping
> -     * to be synced. Still the length needs to follow the address
> -     * alignment changes. Additionally - round the size to the multiple
> -     * of PAGE_SIZE
> -     */
> -    length += ((uintptr_t)addr & (qemu_real_host_page_size() - 1));
> -    length = (length + ~align_mask) & align_mask;
> -
> -    addr = (void *)((uintptr_t)addr & align_mask);
> -
> -    return msync(addr, length, MS_SYNC);
> -#else /* CONFIG_POSIX */
> -    /**
> -     * Perform the sync based on the file descriptor
> -     * The sync range will most probably be wider than the one
> -     * requested - but it will still get the job done
> -     */
> -    return qemu_fdatasync(fd);
> -#endif /* CONFIG_POSIX */
> -}
> -
>  static int64_t suffix_mul(char suffix, int64_t unit)
>  {
>      switch (qemu_toupper(suffix)) {
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index c471c5bc9f8d..161f1123259f 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -950,3 +950,21 @@ int fcntl_setfl(int fd, int flag)
>      }
>      return 0;
>  }
> +
> +int qemu_msync(void *addr, size_t length, int fd)
> +{
> +    size_t align_mask = ~(qemu_real_host_page_size() - 1);
> +
> +    /**
> +     * There are no strict reqs as per the length of mapping
> +     * to be synced. Still the length needs to follow the address
> +     * alignment changes. Additionally - round the size to the multiple
> +     * of PAGE_SIZE
> +     */
> +    length += ((uintptr_t)addr & (qemu_real_host_page_size() - 1));
> +    length = (length + ~align_mask) & align_mask;
> +
> +    addr = (void *)((uintptr_t)addr & align_mask);
> +
> +    return msync(addr, length, MS_SYNC);
> +}
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index f38b06914e12..1e05c316b311 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -596,3 +596,13 @@ size_t qemu_get_host_physmem(void)
>      }
>      return 0;
>  }
> +
> +int qemu_msync(void *addr, size_t length, int fd)
> +{
> +    /**
> +     * Perform the sync based on the file descriptor
> +     * The sync range will most probably be wider than the one
> +     * requested - but it will still get the job done
> +     */
> +    return qemu_fdatasync(fd);
> +}
> -- 
> 2.35.1.693.g805e0a68082a
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|