Re: [Qemu-devel] [PATCH 1/2 v14] fsdev: add IO throttle support to fsdev devices

[Alberto Garcia]

Hello, and sorry for being late reviewing these patches :)

On Tue 24 Jan 2017 10:24:07 AM CET, Pradeep Jagadeesh <address@hidden> wrote:

> This patchset adds the io throttle support for the 9p-local driver.
> For now this functionality can be used only through qemu cli options.
> QMP interface and support to other 9p drivers need further extensions.
> To make it simple for other 9p drivers, the throttle code has been put
> in separate files.

Compared to the previous patch that I reviewed, you removed the
fsdev_throttle_cleanup() call from v9fs_device_unrealize_common(). Why
did you do that?

I miss the list of changes in the cover letter.

> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3520,6 +3520,10 @@ int v9fs_device_realize_common(V9fsState *s, Error 
> **errp)
>          error_setg(errp, "share path %s is not a directory", fse->path);
>          goto out;
>      }
> +
> +    s->ctx.fst = &fse->fst;
> +    fsdev_throttle_init(s->ctx.fst);
> +
>      v9fs_path_free(&path);
>  
>      rc = 0;
> @@ -3528,6 +3532,7 @@ out:
>          if (s->ops && s->ops->cleanup && s->ctx.private) {
>              s->ops->cleanup(&s->ctx);
>          }
> +        fsdev_throttle_cleanup(s->ctx.fst);
>          g_free(s->tag);
>          g_free(s->ctx.fs_root);
>          v9fs_path_free(&path);

You also added the fsdev_throttle_cleanup() here, but look at the code:

       fsdev_throttle_init(s->ctx.fst);

       v9fs_path_free(&path);

       rc = 0;
   out:
       if (rc) {
           /* ... */
           fsdev_throttle_cleanup(s->ctx.fst);
           /* ... */
       }

If rc != 0 you don't need to do the clean up, because there was never an
initialization.

Berto