Re: [Libguestfs] [PATCH nbdkit 3/9] server: Implement Block Status requests to read allocation status.

On 3/19/19 11:35 AM, Richard W.M. Jones wrote: The NBD protocol recommends that context_id for replies to OPT_LIST_META_CONTEXT be set to 0, but requires the client to ignore the context_id. Here, you are blindly returning a context_id of 1 regardless of _LIST or _SET. Hmm. For NBD_CMD_WRITE_ZEROES, we decided that in the common case, .can_zero would default to true for plugins even when the plugin lacks .zero (on the grounds that always advertising it to the client allows for less network traffic, even if nbdkit has to do the fallback) (see commits 7ac98831, bde4fc4). We then added the nozero filter to specifically disable the advertisement as needed. This seems like another case where we should advertise the feature by default (even when the plugin lacks .extents) on the grounds that a client might benefit from knowing that an image can't report sparseness, then provide a filter comparable to nozero that fine-tunes what we actually advertise. But I don't think that fine-tuning affects this patch (which looks right to me to just query .can_extents), but rather what we default to in patch 1 for plugins (I also argued that defaulting filters to default to false until we've audited all filters may be wise). If you take my suggestion of an END type, I think that foreach() should still iterate over any extent marked END, but count_extents should not count it. Or maybe foreach gains a flag for whether the caller is interested in visiting END extents or just stopping right away when one is encountered (filters would be interested, while this code is probably not). If we pre-populate a map with data over the client's initial range and with END beyond the range, it is conceivable that a client will slam the end of the file to be a data extent which will then coalesce to something larger than 32 bits. But we can also be clever and call nbd_extents_add(offset + UINT32_MAX, END) after calling into the client prior to calling foreach to keep this assertion true, even if we want to support the NBD protocol's ability for the last extent (when no REQ_ONE flag) to advertise a length longer that the client's request. You probably ought to return an error if nr_blocks is 0 (possible if a client registered an END type immediately on offset), as the NBD spec requires a successful call to make progress. Hmm. We probably ought to cap the number of extents we are willing to send to avoid an over-long reply. Since we reject NBD_CMD_READ calls requesting more than 64M of data, we probably also ought to guarantee a truncated reply if it would otherwise generate more than 64M of extent information in the reply (or maybe pick a smaller limit, maybe 1M?). Each extent block is 64 bytes, so capping at 1M blocks would be a 64M reply; a lower cap of 64k blocks or even 1k blocks would still be reasonable. Looks good. May need tweaking if you like my idea of having _new() take the client's initial bounds as parameters. Correct observations; permitting arbitrary metadata contexts will indeed be a much more interesting addition, if we ever decide to do so. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org