On Wed, Feb 21, 2024 at 12:19:47PM -0600, Eric Blake wrote:
I noticed during integration testing that nbd-server blindly reports
a
size of 0 for all NBD_OPT_INFO requests, unless I pass a size argument
on the command line to nbd-server. At first, I thought it was a side
effect of me trying to use nbd-server on a block device (an LVM
partition), as it is a common bug to rely on stat().st_size which only
works for regular files (a block device has to use lseek(SEEK_END));
but then I noticed it happening when using nbd-server to serve regular
files as well.
I then turned to the source code, where I see that client->exportsize
is set in just these places:
commit_client()
- exportsize = OFFT_MAX, then try setupexport()
setupexport()
- default to client->server->expected_size (if one was provided),
further validating that actual size is large enough when actual size
can be computed
- if neither treefile or F_WAIT is set, compute actual size by opening
one or more files and using size_autodetect() (which does the right
thing for block devices, so my earlier thought about over-reliance
on stat() was wrong)
but these functions are only reached for NBD_OPT_EXPORT_NAME and
NBD_OPT_GO, not NBD_OPT_INFO.
Yeah, that's a bug. Obviously we don't want to commit_client when we do
OPT_INFO (it's meant to do "everything needed to make sure the export is
ready to go"), but I apparently missed that it also makes sure it has
certain bits of information that OPT_INFO needs.
The upshot is that for NBD_OPT_GO,
there are some scenarios (treefile, F_WAIT) where nbd-server
advertises a size of 9223372036854775807 (0x7fffffff_ffffffff) meaning
unknown, but a size of 0 there is only possible if the file was
successfully opened and really is zero bytes in length. Conversely,
NBD_OPT_INFO is always advertising a size of 0, which means most of
the time, the size changes between NBD_OPT_INFO and NBD_OPT_GO.
That should not happen :)
I think the standard is clear enough that OPT_INFO and OPT_GO should
return the same information. The fact that I messed up doesn't mean I
think we should change the standard :-)
--
w(a)uter.{be,co.za}
wouter(a){grep.be,fosdem.org,debian.org}
I will have a Tin-Actinium-Potassium mixture, thanks.