Re: st_dev and st_ino for pipes

2011-10-05 Thread John Baldwin
On Sunday, October 02, 2011 6:04:05 pm Kostik Belousov wrote:
 Our implementation of pipes does not provide useful values for st_dev
 and st_ino when stat(2) is done on an anonymous pipe. It was noted by the
 people outside the project, e.g. Perl contains a workaround in one
 of its modules, submitted by Debian/kFreeBSD developers, see
 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=537555
 and the commit 16f708c9bc0dc48713b200 in the Perl git.
 
 I think this is a non-conformance, since SUSv4 explicitely states
 in the description of stat(2)
 For all other file types defined in this volume of POSIX.1-2008, the
 structure members st_mode, st_ino, st_dev, st_uid, st_gid, st_atim,
 st_ctim, and st_mtim shall have meaningful values 
 
 Patch below implements the requirement, by the cost of the small overhead
 at the pipe creation time, and slightly bigger cost at the destruction.
 
 Any comments ?

I think this is fine.

-- 
John Baldwin
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org


Re: st_dev and st_ino for pipes

2011-10-04 Thread Peter Jeremy
On 2011-Oct-03 01:04:05 +0300, Kostik Belousov kostik...@gmail.com wrote:
Our implementation of pipes does not provide useful values for st_dev
and st_ino when stat(2) is done on an anonymous pipe. It was noted by the
...
Patch below implements the requirement, by the cost of the small overhead
at the pipe creation time, and slightly bigger cost at the destruction.

Does it need to be so complex?  This information isn't needed by the
kernel and, to be meaningful, all that is required is that the
(st_dev,st_ino) pair is unique within the system.  Given this,
wouldn't it be sufficient to fake up a st_dev and then just make
st_ino be a counter that starts from 0 and increments (atomically?) on
every new pipe?  No need to retain state or free anything when the
pipe is destroyed.  (If necessary, pick a new fake st_dev when st_ino
wraps).

--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
...
+static ino_t pipedev_ino;
..
+  ub-st_dev = pipedev_ino;

st_dev is a dev_t and hence pipedev_ino (which seems misnamed to me)
should probably be dev_t rather than ino_t

-- 
Peter Jeremy


pgpBr4qBgTQzY.pgp
Description: PGP signature


Re: st_dev and st_ino for pipes

2011-10-04 Thread Kostik Belousov
On Tue, Oct 04, 2011 at 05:37:27PM +1100, Peter Jeremy wrote:
 On 2011-Oct-03 01:04:05 +0300, Kostik Belousov kostik...@gmail.com wrote:
 Our implementation of pipes does not provide useful values for st_dev
 and st_ino when stat(2) is done on an anonymous pipe. It was noted by the
 ...
 Patch below implements the requirement, by the cost of the small overhead
 at the pipe creation time, and slightly bigger cost at the destruction.
 
 Does it need to be so complex?  This information isn't needed by the
 kernel and, to be meaningful, all that is required is that the
 (st_dev,st_ino) pair is unique within the system.  Given this,
 wouldn't it be sufficient to fake up a st_dev and then just make
 st_ino be a counter that starts from 0 and increments (atomically?) on
 every new pipe?  No need to retain state or free anything when the
 pipe is destroyed.  (If necessary, pick a new fake st_dev when st_ino
 wraps).
Yes, you described the problem. The (st_dev, st_ino) pair must
be globally unique in system, not only for the pipes, but for whole
domain of file descriptors. This is the reason that I allocate a value
for pipe st_dev using the same devfs mechanism as it is done for device
numbers. Using simple incrementing counter for pipe inodes, together with
allocating yet another st_dev gives essentially the same complications
wrt KPI, and feels unclean.

 
 --- a/sys/kern/sys_pipe.c
 +++ b/sys/kern/sys_pipe.c
 ...
 +static ino_t pipedev_ino;
 ..
 +ub-st_dev = pipedev_ino;
 
 st_dev is a dev_t and hence pipedev_ino (which seems misnamed to me)
 should probably be dev_t rather than ino_t
Fixed.

Thank you.


pgpvSZLW4YilD.pgp
Description: PGP signature


st_dev and st_ino for pipes

2011-10-02 Thread Kostik Belousov
Our implementation of pipes does not provide useful values for st_dev
and st_ino when stat(2) is done on an anonymous pipe. It was noted by the
people outside the project, e.g. Perl contains a workaround in one
of its modules, submitted by Debian/kFreeBSD developers, see
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=537555
and the commit 16f708c9bc0dc48713b200 in the Perl git.

I think this is a non-conformance, since SUSv4 explicitely states
in the description of stat(2)
For all other file types defined in this volume of POSIX.1-2008, the
structure members st_mode, st_ino, st_dev, st_uid, st_gid, st_atim,
st_ctim, and st_mtim shall have meaningful values 

Patch below implements the requirement, by the cost of the small overhead
at the pipe creation time, and slightly bigger cost at the destruction.

Any comments ?

diff --git a/sys/fs/devfs/devfs_devs.c b/sys/fs/devfs/devfs_devs.c
index a2d3222..a111527 100644
--- a/sys/fs/devfs/devfs_devs.c
+++ b/sys/fs/devfs/devfs_devs.c
@@ -171,8 +171,7 @@ devfs_free(struct cdev *cdev)
cdp = cdev2priv(cdev);
if (cdev-si_cred != NULL)
crfree(cdev-si_cred);
-   if (cdp-cdp_inode  0)
-   free_unr(devfs_inos, cdp-cdp_inode);
+   devfs_free_cdp_inode(cdp-cdp_inode);
if (cdp-cdp_maxdirent  0) 
free(cdp-cdp_dirents, M_DEVFS2);
free(cdp, M_CDEVP);
@@ -394,7 +393,7 @@ devfs_delete(struct devfs_mount *dm, struct devfs_dirent 
*de, int flags)
mac_devfs_destroy(de);
 #endif
if (de-de_inode  DEVFS_ROOTINO) {
-   free_unr(devfs_inos, de-de_inode);
+   devfs_free_cdp_inode(de-de_inode);
de-de_inode = 0;
}
if (DEVFS_DE_DROP(de))
@@ -685,6 +684,21 @@ devfs_destroy(struct cdev *dev)
devfs_generation++;
 }
 
+ino_t
+devfs_alloc_cdp_inode(void)
+{
+
+   return (alloc_unr(devfs_inos));
+}
+
+void
+devfs_free_cdp_inode(ino_t ino)
+{
+
+   if (ino  0)
+   free_unr(devfs_inos, ino);
+}
+
 static void
 devfs_devs_init(void *junk __unused)
 {
diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index b7ae521..2edecf2 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
@@ -93,6 +93,7 @@ __FBSDID($FreeBSD$);
 
 #include sys/param.h
 #include sys/systm.h
+#include sys/conf.h
 #include sys/fcntl.h
 #include sys/file.h
 #include sys/filedesc.h
@@ -224,6 +225,8 @@ static int  pipe_zone_init(void *mem, int size, int flags);
 static voidpipe_zone_fini(void *mem, int size);
 
 static uma_zone_t pipe_zone;
+static struct unrhdr *pipeino_unr;
+static ino_t pipedev_ino;
 
 SYSINIT(vfs, SI_SUB_VFS, SI_ORDER_ANY, pipeinit, NULL);
 
@@ -235,6 +238,10 @@ pipeinit(void *dummy __unused)
pipe_zone_ctor, NULL, pipe_zone_init, pipe_zone_fini,
UMA_ALIGN_PTR, 0);
KASSERT(pipe_zone != NULL, (pipe_zone not initialized));
+   pipeino_unr = new_unrhdr(1, INT32_MAX, NULL);
+   KASSERT(pipeino_unr != NULL, (pipe fake inodes not initialized));
+   pipedev_ino = devfs_alloc_cdp_inode();
+   KASSERT(pipedev_ino  0, (pipe dev inode not initialized));
 }
 
 static int
@@ -562,6 +569,12 @@ pipe_create(pipe, backing)
/* If we're not backing this pipe, no need to do anything. */
error = 0;
}
+   if (error == 0) {
+   pipe-pipe_ino = alloc_unr(pipeino_unr);
+   if (pipe-pipe_ino == -1)
+   /* pipeclose will clear allocated kva */
+   error = ENOMEM;
+   }
return (error);
 }
 
@@ -1408,9 +1421,10 @@ pipe_stat(fp, ub, active_cred, td)
ub-st_ctim = pipe-pipe_ctime;
ub-st_uid = fp-f_cred-cr_uid;
ub-st_gid = fp-f_cred-cr_gid;
+   ub-st_dev = pipedev_ino;
+   ub-st_ino = pipe-pipe_ino;
/*
-* Left as 0: st_dev, st_ino, st_nlink, st_rdev, st_flags, st_gen.
-* XXX (st_dev, st_ino) should be unique.
+* Left as 0: st_nlink, st_rdev, st_flags, st_gen.
 */
return (0);
 }
@@ -1463,6 +1477,7 @@ pipeclose(cpipe)
 {
struct pipepair *pp;
struct pipe *ppipe;
+   ino_t ino;
 
KASSERT(cpipe != NULL, (pipeclose: cpipe == NULL));
 
@@ -1521,6 +1536,12 @@ pipeclose(cpipe)
knlist_destroy(cpipe-pipe_sel.si_note);
 
/*
+* Postpone the destroy of the fake inode number allocated for
+* our end, until pipe mtx is unlocked.
+*/
+   ino = cpipe-pipe_ino;
+
+   /*
 * If both endpoints are now closed, release the memory for the
 * pipe pair.  If not, unlock.
 */
@@ -1532,6 +1553,9 @@ pipeclose(cpipe)
uma_zfree(pipe_zone, cpipe-pipe_pair);
} else
PIPE_UNLOCK(cpipe);
+
+   if (ino  0)
+   free_unr(pipeino_unr, cpipe-pipe_ino);
 }
 
 /*ARGSUSED*/
diff --git a/sys/sys/conf.h b/sys/sys/conf.h
index 08e1582..7acd2e0 100644
--- a/sys/sys/conf.h
+++ b/sys/sys/conf.h
@@