Bug 1855330 - pread/.. fails in .prepare if .get_size is not called first in nbdkit-filter
Summary: pread/.. fails in .prepare if .get_size is not called first in nbdkit-filter
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Virtualization Tools
Classification: Community
Component: nbdkit
Version: unspecified
Hardware: All
OS: All
unspecified
unspecified
Target Milestone: ---
Assignee: Richard W.M. Jones
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-07-09 15:04 UTC by wisp3rwind
Modified: 2020-07-22 09:45 UTC (History)
11 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2020-07-22 09:45:12 UTC
Embargoed:


Attachments (Terms of Use)

Description wisp3rwind 2020-07-09 15:04:55 UTC
Description of problem:

When calling `next_ops->pread(nxdata, ...)` from the .prepare method of an nbdkit-filter without calling `next_ops->get_size(nxdata)` first, the assertion

    assert (h->exportsize <= INT64_MAX); /* Guaranteed by negotiation phase */

in nbdkit/server/backend.c/backend_valid_range() will fail, resulting in an error message which is hard to understand. The "Guaranteed by negotiation phase" comment wasn't really helpful either, what did lead to the solution was reading more of the source code and git blame. In fact, the documentation for .prepare in `man nbdkit-filter` suggests that calling `next_ops->pread` should simply work.

Maybe this expected, in which case I think documentation in `man nbdkit-filter` should mention the need to call `get_size()` manually.

Comment 1 Eric Blake 2020-07-09 15:19:18 UTC
The check exists because pread wants to ensure the request is in-bounds, with a sane error reported if not, but get_size failure does not guarantee a sane error return.  So yes, I'm leaning towards this being a documentation flaw.

Since all of our filters are in-tree, it's not hard to update them to whatever semantics we find easiest - if we think .pread should just work (by calling .get_size as needed), we could do that.  But merely documenting that all filters must ensure .get_size has been called first (which is automatic in most cases, but tricky in .prepare), is reasonable.

Comment 2 Richard W.M. Jones 2020-07-09 15:20:24 UTC
See also:
https://www.redhat.com/archives/libguestfs/2020-July/msg00041.html

I don't think we can remove the restriction, since get_size
really must be called (either by .prepare or .get_size of the filter),
but we can probably make the error easier to understand and/or
document the problem.

Comment 3 Eric Blake 2020-07-09 16:38:25 UTC
patch proposed:
https://www.redhat.com/archives/libguestfs/2020-July/msg00062.html

Comment 4 Richard W.M. Jones 2020-07-22 09:45:12 UTC
Fixed upstream (by documentation) in:
https://github.com/libguestfs/nbdkit/commit/1030401ea438f83e68696a7e141c08e49bfde104


Note You need to log in before you can comment on or make changes to this bug.