<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Jul 30, 2018 at 5:45 PM Eric Blake <<a href="mailto:eblake@redhat.com">eblake@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 07/28/2018 06:42 AM, Nir Soffer wrote:<br>
> Fix issues Eric found in the original patch:<br>
> <a href="https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html" rel="noreferrer" target="_blank">https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html</a><br>
> <br>
> - When handling ENODEV, the caller is expecting ENOTSUPP to trigger<br>
>    fallback.<br>
> - ENODEV should be ignored in file_trim.<br>
> <br>
> Tested only on Fedora 28.<br>
> ---<br>
>   plugins/file/file.c | 33 ++++++++++++++++++++++++---------<br>
>   1 file changed, 24 insertions(+), 9 deletions(-)<br>
> <br>
> diff --git a/plugins/file/file.c b/plugins/file/file.c<br>
> index a7c07fb..4210adb 100644<br>
> --- a/plugins/file/file.c<br>
> +++ b/plugins/file/file.c<br>
> @@ -50,6 +50,21 @@<br>
>   <br>
>   static char *filename = NULL;<br>
>   <br>
> +#if defined(FALLOC_FL_PUNCH_HOLE) || defined(FALLOC_FL_ZERO_RANGE)<br>
> +static int<br>
> +do_fallocate(int fd, int mode, off_t offset, off_t len)<br>
> +{<br>
<br>
Similar name to the normalizing wrapper that qemu uses, but enough <br>
differences that I think you're safe here. (Be careful that you aren't <br>
directly copying code from that project to this one, due to the <br>
difference in licensing).<br>
<br>
> +  int r = -1;<br>
> +  r = fallocate (fd, mode, offset, len);<br>
> +  /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9 fails<br>
> +     with EOPNOTSUPP in this case. Normlize errno to simplify callers. */<br>
<br>
s/Normlize/Normalize/ </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +  if (r == -1 && errno == ENODEV) {<br>
> +    errno = EOPNOTSUPP;<br>
> +  }<br>
<br>
Question - do we care about retrying on EINTR?  That would also fit well <br>
in this normalizing wrapper.  But that can be a separate patch.<br></blockquote><div><br></div><div>I agree.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
With the typo fix, I'm okay with this patch fixing up the immediate <br>
issues. There's still the bigger question of whether we need to revisit <br>
the logic anyways (using _just_ PUNCH_HOLE when we want to guarantee <br>
zeroes and permit unmap, and _just_ ZERO_RANGE when we want to guarantee <br>
zeroes without discarding) - but that was pre-existing and didn't <br>
regress as a result of the immediate issue that this is trying to fix.<br></blockquote><div><br></div><div>I'm not sure that we can use _just_ something because of the compatibility</div><div>issues with older kernel and block devices. I hope this is improved in</div><div><a href="https://www.redhat.com/archives/libguestfs/2018-July/msg00084.html">https://www.redhat.com/archives/libguestfs/2018-July/msg00084.html</a></div><div><br></div><div>I'll send v2 with the typo fix, we need to fix the regression in the previous</div><div>patch first.</div><div> </div><div>Nir</div></div></div>