On 01.08.19 16:02, Vladimir Sementsov-Ogievskiy wrote: > 31.07.2019 15:09, Max Reitz wrote:
[...] >> So -- without having tried, of course -- I think a better design would >> be to look for bs->file->bs in the ReopenQueue, recursively all of its >> children, and move all of those entries into a new queue, and then >> invoke bdrv_reopen_multiple() on that one first. > > Why all children recursively? Shouldn't we instead only follow defined > dependencies? > Or it just seems bad to put not full subtree into bdrv_reopen multiple() ? For example, making a node RW without opening its children RW seems wrong. Whenever we reopen a node, we should reopen all of its children, if they are in the queue. >> The question then becomes how to roll back those changes... I don’t >> know whether just having bdrv_reopen() partially succeed is so bad. >> Otherwise, we’d need a function to translate an existing node's state >> into a BdrvReopenQueueEntry so we can thus return to the old state. > > And this rollback may fail too and we are still in partial success state. > > But if we roll-back simple ro->rw reopen it's safe enough: in worst case > file will be rw, but marked ro, so Qemu may have more access rights than > needed but will not use them, this is why I was concentrating around > only ro->rw case.. In practice, this is always so. The “children need to be reopened before parent” case is always a sign of more permissions being taken; whereas “children need to be reopened after parent” is a sign of permissions being released. What we want to fix now is the former “reopen children before parent” case. Because this is always a sign of taking more permissions, a partial success/failure state means we always have taken more permissions than we need to. > So, what about go similar way to this patch, i.e. only reopen ro->rw children > which we need to be rw, not touching other flags, but check, that in reopen > queue we have this child, and it is going to be reopened RW, and if not, > return error immediately? If the RO -> RW change for the child is accompanied by other options being changed, the user may find it vital to change these flags along with the RO/RW access. We shouldn’t ignore them. Max
signature.asc
Description: OpenPGP digital signature
