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

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).

> > 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(a)redhat.com&gt; Thanks.