On 07/28/2018 06:42 AM, Nir Soffer wrote:
> Fix issues Eric found in the original patch:
> https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html
>
> - When handling ENODEV, the caller is expecting ENOTSUPP to trigger
>    fallback.
> - ENODEV should be ignored in file_trim.
>
> Tested only on Fedora 28.
> ---
>   plugins/file/file.c | 33 ++++++++++++++++++++++++---------
>   1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/plugins/file/file.c b/plugins/file/file.c
> index a7c07fb..4210adb 100644
> --- a/plugins/file/file.c
> +++ b/plugins/file/file.c
> @@ -50,6 +50,21 @@
>   
>   static char *filename = NULL;
>   
> +#if defined(FALLOC_FL_PUNCH_HOLE) || defined(FALLOC_FL_ZERO_RANGE)
> +static int
> +do_fallocate(int fd, int mode, off_t offset, off_t len)
> +{

Similar name to the normalizing wrapper that qemu uses, but enough
differences that I think you're safe here. (Be careful that you aren't
directly copying code from that project to this one, due to the
difference in licensing).

> +  int r = -1;
> +  r = fallocate (fd, mode, offset, len);
> +  /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9 fails
> +     with EOPNOTSUPP in this case. Normlize errno to simplify callers. */

s/Normlize/Normalize/ 

> +  if (r == -1 && errno == ENODEV) {
> +    errno = EOPNOTSUPP;
> +  }

Question - do we care about retrying on EINTR?  That would also fit well
in this normalizing wrapper.  But that can be a separate patch.

With the typo fix, I'm okay with this patch fixing up the immediate
issues. There's still the bigger question of whether we need to revisit
the logic anyways (using _just_ PUNCH_HOLE when we want to guarantee
zeroes and permit unmap, and _just_ ZERO_RANGE when we want to guarantee
zeroes without discarding) - but that was pre-existing and didn't
regress as a result of the immediate issue that this is trying to fix.