Re: [PATCH 2/4] add chdir-notify API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 28, 2018 at 01:58:18PM -0400, Eric Sunshine wrote:

> On Wed, Mar 28, 2018 at 1:40 PM, Jeff King <peff@xxxxxxxx> wrote:
> > [...]
> > Let's provide an API to let code that stores relative paths
> > "subscribe" to updates to the current working directory.
> > This means that callers of chdir() don't need to know about
> > all subscribers ahead of time; they can simply consult a
> > dynamically built list.
> > [...]
> > Signed-off-by: Jeff King <peff@xxxxxxxx>
> > ---
> > diff --git a/chdir-notify.c b/chdir-notify.c
> > @@ -0,0 +1,71 @@
> > +int chdir_notify(const char *new_cwd)
> > +{
> > +       struct strbuf old_cwd = STRBUF_INIT;
> > +       struct list_head *pos;
> > +
> > +       if (strbuf_getcwd(&old_cwd) < 0)
> > +               return -1;
> > +       if (chdir(new_cwd) < 0)
> > +               return -1;
> 
> This 'return' is leaking 'old_cwd', isn't it?

Whoops, yes. I wrote it the other way around first to avoid the leak,
but of course that has other problems. ;)

> > diff --git a/chdir-notify.h b/chdir-notify.h
> > @@ -0,0 +1,64 @@
> > + * In practice most callers will want to move a relative path to the new root;
> > + * they can use the reparent_relative_path() helper for that. If that's all
> > + * you're doing, you can also use the convenience function:
> > + *
> > + *   chdir_notify_reparent(&my_path);
> > + */
> > +typedef void (*chdir_notify_callback)(const char *old_cwd,
> > +                                     const char *new_cwd,
> > +                                     void *data);
> > +void chdir_notify_register(chdir_notify_callback cb, void *data);
> > +void chdir_notify_reparent(char **path);
> 
> Can we have some documentation here (or in the chdir_notify_reparent()
> example above) explaining that *path is a heap-allocated value? I had
> to consult the implementation to understand ownership.

Sure. I originally had reparent_relative_path() handle the freeing and
allocation, and it explained the ownership in its docstring. But as I
revised that to simply return its value, that got lost. I'll add it back
in here.

-Peff



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux