Re: mountlist: libmount support on Linux

[Karel Zak]

On Wed, Aug 27, 2014 at 01:06:02PM -0600, Eric Blake wrote:
> On 08/27/2014 10:15 AM, Pádraig Brady wrote:
> > On 08/27/2014 03:08 PM, Fridolin Pokorny wrote:
> >> diff --git a/lib/mountlist.c b/lib/mountlist.c
> > 
> > Cool, this fits well.
> > It would be good to mention the functionality and
> > performance benefits in the changelog.
> > 
> >> +#ifdef MOUNTED_PROC_MOUNTINFO
> >> +static const char *
> >> +mountinfo_path (void) {
> >> +  static char filename[sizeof ("/proc//mountinfo") + 13]; /* 13 to hold a 
> >> PID */
> 
> The magic number 13 is gross, compared to using <intprops.h> from gnulib
> and using INT_STRLEN_BOUND(pid_t) (which should evaluate to 13) instead.
> 
> >> +
> >> +  sprintf (filename, "/proc/%u/mountinfo", getpid ());
> >> +
> >> +  return filename;
> >> +}
> > 
> > I dislike the above as it precludes concurrent usage.
> > You could avoid that issue by allocating on the heap,
> 
> or by setting up a witness so that sprintf is called only once by the
> first caller, and all later callers wait until the first caller is complete.
> 
> > but can't this be simplified by using /proc/self/mountinfo ?
> 
> The argument here was that the kernel is slightly slower in order to
> resolve the symlink of /proc/self in all cases - but is that penalty
> going to be noticeable?

 Well, we have /proc/self is everywhere, including libmount, findmnt, 
 mount, ... systemd sources:

   $ git grep "/proc/self" | wc -l
   51

 please, don't try to be more smart then kernel, it's premature
 optimization to care about such things in userspace.

 
 BTW, there will be /proc/thread-self
 (http://lwn.net/Articles/607422/)


    Karel

-- 
 Karel Zak  <address@hidden>
 http://karelzak.blogspot.com