[Libguestfs] [libnbd PATCH] copy: Fail nbdcopy if NBD read or write fails

[Laszlo Ersek]

On 02/03/22 15:12, Eric Blake wrote:
> On Thu, Feb 03, 2022 at 02:46:27PM +0200, Nir Soffer wrote:

>> So we would use fprintf and strerror, and setting errno would be needed
>> although it can be used with %m (is it portable?).
> 
> %m is not portable in *printf(3).  It IS portable in syslog(3), which
> is why glibc printf(3) supports %m as well; and in nbdkit, we use a
> printf wrapper on non-glibc to give unconditional %m support to nbdkit
> plugins.  But we do not have that printf wrapper here.  Using
> strerror() instead of perror() is doable in applications (we shouldn't
> do it directly in libnbd since strerror() has multi-threaded
> implications and we don't control what the application may be doing in
> other threads, but in nbdcopy we control the entire application).

Side topic: what do we think about strerror_r()?

... Hmmm I recall something... Yep: we now have guestfs_int_strerror()
in the libguestfs-common project at least.

(We cannot just copy it over I think, due to licence differences;
however, with Rich having authored commit 9ec7cd8e23b5 ("utils: Fix
usage of strerror_r", 2021-12-09), Red Hat seems to "own the copyright
on that function" (if this statement makes any sense), so we could
"relicense" it.)

Thanks,
Laszlo

> 
>>
>> For now I think this is fine, we want a minimal change for easy backport.
> 
> Yep, and since I'm the one that will be doing the backports, I'm aware
> of the problems of making the patch too complex ;)
> 
>>
>>> +
>>>    if (allocated || sparse_size == 0) {
>>>      /* If sparseness detection (see below) is turned off then we write
>>>       * the whole command.
>>> @@ -475,6 +482,7 @@ finished_read (void *vp, int *error)
>>>      /* Free the original command since it has been split into
>>>       * subcommands and the original is no longer needed.
>>>       */
>>> +    errno = 0;
>>>      free_command (command, &errno);
>>
>> Using free_command as a callback is ugly, but good enough for now.
> 
> Is your complaint that in some places we call free_command() directly,
> in other places, we rely on libnbd to call it as a callback?  Yeah, it
> can get confusing, and it took me a while to convince myself that we
> call it exactly once on all call paths, particularly when asynch_zero
> returns true vs. false.
> 
>>
>> Would be good to test also writing zeroes, but otherwise the fix is complete.
> 
> Given that I will be revising the patch to add in a CVE number
> anyways, I'll improve the test for v2.
> 
>>
>> Reviewed-by: Nir Soffer <nsoffer at redhat.com>
> 
> Thanks.
>