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