01.08.2019 22:06, Max Reitz wrote: > 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.
OK, I'll try. > >> 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. > Hmm, understand, OK. -- Best regards, Vladimir
