On Wed, Aug 1, 2012 at 10:53 AM, Ben Pfaff <b...@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 <b...@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 <b...@nicira.com> wrote:
> > > >
> > > > > It will acquire its first user in an upcoming commit.
> > > > >
> > > > > Signed-off-by: Ben Pfaff <b...@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.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
