Re: [PATCH v2 00/10] block: Attempt on fixing 030-reported errors

[Kevin Wolf]

Am 11.11.2021 um 13:08 hat Hanna Reitz geschrieben:
> Hi,
> 
> v1 cover letter:
> https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg01287.html
> 
> In v2 I’ve addressed the comments I’ve received from Kevin and Vladimir.
> To this end, I’ve retained only the non-controversial part in patch 5,
> and split everything else (i.e. the stuff relating to
> bdrv_replace_child_tran()) into the dedicated patches 6 and 8.
> 
> Kevin’s most important comments (to my understanding) were that:
> 
> (A) When bdrv_remove_file_or_backing_child() uses
>     bdrv_replace_child_tran(), we have to ensure that the BDS lives as
>     long as the transaction does.  This is solved by keeping a strong
>     reference to it that’s released only when the transaction is cleaned
>     up (and the new patch 7 ensures that the .clean() handler will be
>     invoked after all .commit()/.abort() handlers, so the reference will
>     really live as long as the transaction does).
> 
> (B) bdrv_replace_node_noperm() passes a pointer to loop-local variable,
>     which is a really bad idea considering the transaction lives much
>     longer than one loop iteration.
>     Luckily, the new_bs it passes is always non-NULL, and so
>     bdrv_replace_child_tran() actually doesn’t need to store the
>     BdrvChild ** pointer, because for a non-NULL new_bs, there is
>     nothing to revert in the abort handler.  We just need to clarify
>     this, not store the pointer in case of a non-NULL new_bs, and assert
>     that bdrv_replace_node_noperm() and its relatives are only used to
>     replace an existing node by some other existing (i.e. non-NULL)
>     node.

Thanks, applied to the block branch.

Kevin