Re: [PATCH 0/7] block: Attempt on fixing 030-reported errors

[Kevin Wolf <kwolf@redhat.com>]

Am 04.11.2021 um 11:38 hat Hanna Reitz geschrieben:
> (2A) bdrv_replace_child_noperm() should immediately set bs->file or
>      bs->backing to NULL when it sets bs->{file,backing}->bs to NULL.
>      It should also immediately remove any BdrvChild with .bs == NULL
>      from the parent’s BDS.children list.
>      Implemented in patches 2 through 6.
> 
> (2B) Alternatively, we could always keep the whole subgraph drained
>      while we manipulate it.  Then, the bdrv_parent_drained_end_single()
>      in bdrv_replace_child_noperm() wouldn’t do anything.
>      To fix 030, we would need to add a drained section to
>      stream_prepare(): Namely we’d need to drain the subgraph below the
>      COR filter node.
>      This would be a much simpler solution, but I don’t feel like it’s
>      the right one.

> As you can see, I’m not sure which of 2A or 2B is the right solution.  I
> decided to investigate both: 2A was much more complicated, but seemed
> like the right thing to do; 2B is much simpler, but doesn’t feel as
> right.  Therefore, I decided to go with 2A in this first version of this
> series.

I haven't looked at the patches yet, but if I understand correctly the
choice you're presenting here is between protecting code from accessing
invalid state and not creating the invalid state in the first place. I
agree that the latter is preferable as long as it doesn't make things so
complicated that we would be willing to accept the higher risk of
breakage in the former. If it's doable in five patches, it's probably
not complicated enough to make such compromises.

Kevin