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

[Pradeep Jagadeesh]

On 2/1/2017 4:08 PM, Alberto Garcia wrote:
> 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 actually missed while merging the code to newer version. Instead of 
putting inside the unrealize it went some other function.

> 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.
Fixed it, it was because of merging issue as I answered above.

-Pradeep
> Berto