[Libguestfs] [PATCH nbdkit] xz: Do not pass can_write through to the plugin.

[Eric Blake]

On 1/30/19 8:36 AM, Richard W.M. Jones wrote:
> I'm not sure that this fix is really correct.

The fix is correct in the sense that yes, the xz filter cannot allow
writes, and must therefore override write support from leaking through
from the plugin.

> 
> An alternate way I can think to fix this would be for the core server
> to maintain a readonly flag for each layer (instead of just per-
> server).
> 
> You could also argue that our readonly test in
> server/connections.c:compute_eflags is wrong and/or that the
> implementations of server/plugins.c:plugin_can_write and
> server/filters.c:filter_can_write have the wrong default - they should
> consult the readonly flag which was passed to that layer's open call.

Arguing that filters.c picks the wrong defaults makes a bit of intuitive
sense, but it trickier to implement correctly.

We have several examples of filters that override .pwrite but not
.can_write (e.g. truncate), some that override .can_write but not
.pwrite (xz), some that override both (cow), some that override neither
(nozero).  When .can_write is overridden, it is obviously correct. But
when .can_write is absent, whether or not .pwrite is also absent, I can
see your idea of defaulting to "if I passed readonly=true to the
plugin's open(), synthesize .can_write of false no matter what; if I
passed readonly=false to the plugin's open(), then call the plugin's
.can_write() which may return true or false based on other aspects up to
the plugin".

And we can track what the filter passes to the plugin's .open, since a
filter's .open MUST call the next(nxdata) parameters it was given. Since
next(nxdata) is implemented in filters.c, THAT could be the place where
we stash off whether the filter is passing true/false to the underling
plugin's .open (even if that value differs from the value of readonly
passed to the filter's .open), such that we can then have filters.c in
charge of remembering the per-connection state of whether the underlying
plugin is in forced-readonly mode and therefore the filter must
synthesize a .can_read that forces false.

> 
> However the fix only affects one filter, and we're not maintaining an
> API for filters, so maybe we can defer the problem.

I agree that our filter API is not stable, so I'm okay with your patch
going in now, regardless of whether we also change filters.c to have
saner defaults.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190130/f79a9853/attachment.sig>