Re: [pve-devel] [PATCH v2 qemu 1/2] vma: restore: call blk_unref for all opened block devices

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

lgtm

While looking at the code again, I do find the error handling in
`extract_content` generally a bit lacking, though, eg. the `blk` var
moved down below is only set if `errp` is not set, but we still go ahead
with everything else, making me wonder if we shouldn't bail out then
instead...

But that's out of scope of this patch, this definitely fixes what
appears to be buggy cleanup code.

On Thu, Apr 21, 2022 at 01:26:47PM +0200, Fabian Ebner wrote:
> rather than just the last one. This is the only caller registering
> block devices with the reader, so other callers are already fine.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v2.
> 
> Best squashed into 0026-PVE-Backup-add-vma-backup-format-code.patch
> 
>  vma-reader.c | 3 +++
>  vma.c        | 6 ++----
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/vma-reader.c b/vma-reader.c
> index 2b1d1cdab3..4f4ee2b47b 100644
> --- a/vma-reader.c
> +++ b/vma-reader.c
> @@ -192,6 +192,9 @@ void vma_reader_destroy(VmaReader *vmar)
>          if (vmar->rstate[i].bitmap) {
>              g_free(vmar->rstate[i].bitmap);
>          }
> +        if (vmar->rstate[i].target) {
> +            blk_unref(vmar->rstate[i].target);
> +        }
>      }
>  
>      if (vmar->md5csum) {
> diff --git a/vma.c b/vma.c
> index df542b7732..89440733b1 100644
> --- a/vma.c
> +++ b/vma.c
> @@ -309,8 +309,6 @@ static int extract_content(int argc, char **argv)
>      int vmstate_fd = -1;
>      guint8 vmstate_stream = 0;
>  
> -    BlockBackend *blk = NULL;
> -
>      for (i = 1; i < 255; i++) {
>          VmaDeviceInfo *di = vma_reader_get_device_info(vmar, i);
>          if (di && (strcmp(di->devname, "vmstate") == 0)) {
> @@ -331,6 +329,8 @@ static int extract_content(int argc, char **argv)
>              int flags = BDRV_O_RDWR;
>              bool write_zero = true;
>  
> +            BlockBackend *blk = NULL;
> +
>              if (readmap) {
>                  RestoreMap *map;
>                  map = (RestoreMap *)g_hash_table_lookup(devmap, di->devname);
> @@ -443,8 +443,6 @@ static int extract_content(int argc, char **argv)
>  
>      vma_reader_destroy(vmar);
>  
> -    blk_unref(blk);
> -
>      bdrv_close_all();
>  
>      return ret;
> -- 
> 2.30.2