[ovs-dev] [debian 7/9] util: New function follow_symlinks().

[Ansis Atteka]

On Wed, Aug 1, 2012 at 10:53 AM, Ben Pfaff <blp at nicira.com> wrote:

> On Wed, Aug 01, 2012 at 10:32:59AM -0700, Ansis Atteka wrote:
> > On Wed, Aug 1, 2012 at 10:11 AM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > > On Tue, Jul 31, 2012 at 10:06:33AM -0700, Ansis Atteka wrote:
> > > > On Mon, Jul 30, 2012 at 3:18 PM, Ben Pfaff <blp at nicira.com> wrote:
> > > >
> > > > > It will acquire its first user in an upcoming commit.
> > > > >
> > > > > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > >
> > > ...
> > >
> > > > > +xreadlink(const char *filename)
> > > > > +{
> > > > > +    size_t size;
> > > > >
> > > > +
> > > > > +    for (size = 64; ; size *= 2) {
> > > > > +        char *buf = xmalloc(size);
> > > > > +        int retval = readlink(filename, buf, size);
> > > > > +        int error = errno;
> > > > > +
> > > > > +        if (retval >= 0 && retval < size) {
> > > > > +            buf[retval] = '\0';
> > > > > +            return buf;
> > > > > +        }
> > > > > +
> > > > > +        free(buf);
> > > > > +        if (retval < size) {
> > > > >
> > > > Shouldn't size be of type ssize_t instead? Otherwise this could loop
> > > > forever, if readlink() returned -1.
> > >
> > > Good catch.  Thank you, that's a nasty bug.
> > >
> > > I think that changing (retval < size) to (retval < 0) fixes the
> > > problem too.  What do you think?
> > >
> > Yes, comparing with "0" actually would be better.
>
> Thanks, changed.
>
> > Also, I guess the type of retval should be ssize_t instead of int? I
> guess
> > this was changed starting from SUSv3.
>
> Thanks, changed.
>
> > Did you consider to use constant PATH_MAX or SYMLINK_MAX? Anyway,
> > I am completely fine with your solution too, because I think that the
> > path in majority of cases would fit in a 64 byte buffer anyway.
>
> PATH_MAX is always tempting, but POSIX says:
>
>     The values in the following list may be constants within an
>     implementation or may vary from one pathname to another. For
>     example, file systems or directories may have different
>     characteristics.
>
>     A definition of one of the symbolic constants in the following
>     list shall be omitted from the <limits.h> header on specific
>     implementations where the corresponding value is equal to or
>     greater than the stated minimum, but where the value can vary
>     depending on the file to which it is applied. The actual value
>     supported for a specific pathname shall be provided by the
>     pathconf() function.
>
> which means that you end up having to call pathconf() in some cases.
> And some systems don't really have a limit anyway.
>
> GNU Hurd is an example of a system where PATH_MAX causes problems:
>
> http://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html
>
> In the extreme, you end up with nastiness like this:
>         http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/pathmax.h
>
> So I prefer to just use dynamic allocation, because to me it seems
> cleaner.
>
Ok. Assuming that you addressed those two other things, the
patch looks good to me. Thanks!

>
> Thanks,
>
> Ben.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20120801/9e32b611/attachment-0003.html>