[PATCH] nbd: show nbd client pid in sysfs

[PATCH] nbd: show nbd client pid in sysfs

[Paul Clements]

[-- Attachment #1: Type: text/plain, Size: 252 bytes --]

This simple patch allows nbd to expose the nbd-client daemon's PID in 
/sys/block/nbd<x>/pid. This is helpful for tracking connection status of 
a device and for determining which nbd devices are currently in use.

Tested against 2.6.19.

Thanks,
Paul

[-- Attachment #2: nbd_pid_sysfs.diff --]
[-- Type: text/plain, Size: 1019 bytes --]

--- ./drivers/block/nbd.c	Wed Nov 29 16:57:37 2006
+++ ./drivers/block/nbd.c	Tue Nov 28 16:09:31 2006
@@ -355,14 +389,30 @@ harderror:
 	return NULL;
 }
 
+static ssize_t pid_show(struct gendisk *disk, char *page)
+{
+	return sprintf(page, "%ld\n",
+		(long) ((struct nbd_device *)disk->private_data)->pid);
+}
+
+static struct disk_attribute pid_attr = {
+	.attr = { .name = "pid", .mode = S_IRUGO },
+	.show = pid_show,
+};
+	
 static void nbd_do_it(struct nbd_device *lo)
 {
 	struct request *req;
 
 	BUG_ON(lo->magic != LO_MAGIC);
 
+	lo->pid = current->pid;
+	sysfs_create_file(&lo->disk->kobj, &pid_attr.attr);
+
 	while ((req = nbd_read_stat(lo)) != NULL)
 		nbd_end_request(req);
+
+	sysfs_remove_file(&lo->disk->kobj, &pid_attr.attr);
 	return;
 }
 
--- ./include/linux/nbd.h	Wed Nov 29 16:57:37 2006
+++ ./include/linux/nbd.h	Mon Dec  4 23:28:30 2006
@@ -64,6 +64,7 @@ struct nbd_device {
 	struct gendisk *disk;
 	int blksize;
 	u64 bytesize;
+	pid_t pid; /* pid of nbd-client, if attached */
 };
 
 #endif

Re: [PATCH] nbd: show nbd client pid in sysfs

[Pavel Machek]

Hi!

> This simple patch allows nbd to expose the nbd-client 
> daemon's PID in /sys/block/nbd<x>/pid. This is helpful 
> for tracking connection status of a device and for 
> determining which nbd devices are currently in use.

Should get a line or two in Documentation/ ?
						Pavel

> Tested against 2.6.19.
> 
> Thanks,
> Paul

> --- ./drivers/block/nbd.c	Wed Nov 29 16:57:37 2006
> +++ ./drivers/block/nbd.c	Tue Nov 28 16:09:31 2006
> @@ -355,14 +389,30 @@ harderror:
>  	return NULL;
>  }
>  
> +static ssize_t pid_show(struct gendisk *disk, char *page)
> +{
> +	return sprintf(page, "%ld\n",
> +		(long) ((struct nbd_device *)disk->private_data)->pid);
> +}
> +
> +static struct disk_attribute pid_attr = {
> +	.attr = { .name = "pid", .mode = S_IRUGO },
> +	.show = pid_show,
> +};
> +	
>  static void nbd_do_it(struct nbd_device *lo)
>  {
>  	struct request *req;
>  
>  	BUG_ON(lo->magic != LO_MAGIC);
>  
> +	lo->pid = current->pid;
> +	sysfs_create_file(&lo->disk->kobj, &pid_attr.attr);
> +
>  	while ((req = nbd_read_stat(lo)) != NULL)
>  		nbd_end_request(req);
> +
> +	sysfs_remove_file(&lo->disk->kobj, &pid_attr.attr);
>  	return;
>  }
>  
> --- ./include/linux/nbd.h	Wed Nov 29 16:57:37 2006
> +++ ./include/linux/nbd.h	Mon Dec  4 23:28:30 2006
> @@ -64,6 +64,7 @@ struct nbd_device {
>  	struct gendisk *disk;
>  	int blksize;
>  	u64 bytesize;
> +	pid_t pid; /* pid of nbd-client, if attached */
>  };
>  
>  #endif


-- 
Thanks for all the (sleeping) penguins.

Re: [PATCH] nbd: show nbd client pid in sysfs

[Pavel Machek]

Hi!

> This simple patch allows nbd to expose the nbd-client 
> daemon's PID in /sys/block/nbd<x>/pid. This is helpful 
> for tracking connection status of a device and for 
> determining which nbd devices are currently in use.

Actually is it needed at all? Perhaps nbd clients should be modified
to put nbdX in their process nam?

> 
> Tested against 2.6.19.
> 
> Thanks,
> Paul

> --- ./drivers/block/nbd.c	Wed Nov 29 16:57:37 2006
> +++ ./drivers/block/nbd.c	Tue Nov 28 16:09:31 2006
> @@ -355,14 +389,30 @@ harderror:
>  	return NULL;
>  }
>  
> +static ssize_t pid_show(struct gendisk *disk, char *page)
> +{
> +	return sprintf(page, "%ld\n",
> +		(long) ((struct nbd_device *)disk->private_data)->pid);
> +}
> +
> +static struct disk_attribute pid_attr = {
> +	.attr = { .name = "pid", .mode = S_IRUGO },
> +	.show = pid_show,
> +};
> +	
>  static void nbd_do_it(struct nbd_device *lo)
>  {
>  	struct request *req;
>  
>  	BUG_ON(lo->magic != LO_MAGIC);
>  
> +	lo->pid = current->pid;
> +	sysfs_create_file(&lo->disk->kobj, &pid_attr.attr);
> +
>  	while ((req = nbd_read_stat(lo)) != NULL)
>  		nbd_end_request(req);
> +
> +	sysfs_remove_file(&lo->disk->kobj, &pid_attr.attr);
>  	return;
>  }
>  
> --- ./include/linux/nbd.h	Wed Nov 29 16:57:37 2006
> +++ ./include/linux/nbd.h	Mon Dec  4 23:28:30 2006
> @@ -64,6 +64,7 @@ struct nbd_device {
>  	struct gendisk *disk;
>  	int blksize;
>  	u64 bytesize;
> +	pid_t pid; /* pid of nbd-client, if attached */
>  };
>  
>  #endif


-- 
Thanks for all the (sleeping) penguins.

Re: [PATCH] nbd: show nbd client pid in sysfs

[Wouter Verhelst]

On Fri, Dec 08, 2006 at 09:17:23PM +0000, Pavel Machek wrote:
> Hi!
> 
> > This simple patch allows nbd to expose the nbd-client 
> > daemon's PID in /sys/block/nbd<x>/pid. This is helpful 
> > for tracking connection status of a device and for 
> > determining which nbd devices are currently in use.
> 
> Actually is it needed at all? Perhaps nbd clients should be modified
> to put nbdX in their process nam?

I don't think that's the right approach; only the kernel can guarantee
that a given process is actually managing a given nbd device (I could
have some rogue process running around announcing that it's managing
nbd2, and then what?)

-- 
<Lo-lan-do> Home is where you have to wash the dishes.
  -- #debian-devel, Freenode, 2004-09-22

Re: [PATCH] nbd: show nbd client pid in sysfs

[Pavel Machek]

Hi!

> > > This simple patch allows nbd to expose the nbd-client 
> > > daemon's PID in /sys/block/nbd<x>/pid. This is helpful 
> > > for tracking connection status of a device and for 
> > > determining which nbd devices are currently in use.
> > 
> > Actually is it needed at all? Perhaps nbd clients should be modified
> > to put nbdX in their process nam?
> 
> I don't think that's the right approach; only the kernel can guarantee
> that a given process is actually managing a given nbd device (I could
> have some rogue process running around announcing that it's managing
> nbd2, and then what?)

I'd say "do not run rogue processes as root" :-).

nbd-client should run as root -- I do not think interface was
carefully audited to run it as a user -- so rogue process should not
really be a problem.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] nbd: show nbd client pid in sysfs

[Wouter Verhelst]

On Sun, Dec 10, 2006 at 08:58:19PM +0100, Pavel Machek wrote:
> Hi!

Hi

> > > > This simple patch allows nbd to expose the nbd-client 
> > > > daemon's PID in /sys/block/nbd<x>/pid. This is helpful 
> > > > for tracking connection status of a device and for 
> > > > determining which nbd devices are currently in use.
> > > 
> > > Actually is it needed at all? Perhaps nbd clients should be modified
> > > to put nbdX in their process nam?
> > 
> > I don't think that's the right approach; only the kernel can guarantee
> > that a given process is actually managing a given nbd device (I could
> > have some rogue process running around announcing that it's managing
> > nbd2, and then what?)
> 
> I'd say "do not run rogue processes as root" :-).
> 
> nbd-client should run as root -- I do not think interface was
> carefully audited to run it as a user -- so rogue process should not
> really be a problem.

IOW, you're suggesting I walk the process list from userspace to find a
process for which a) it claims it's running for a given nbd device, and
b) I can verify that it actually has the permissions to do so. That's a
whole lot of code in comparison to

f=open("/sys/block/nbd2/pid", O_RDONLY);
read(f,buf,len);

I think I very much prefer the above two lines, not only for simplicity.

Also, your suggestion relies on users /only/ using the official
nbd-client, and is fragile in cases where that assumption is false
(i.e., it's susceptible to false negatives). The suggested patch does
not have that problem.

-- 
<Lo-lan-do> Home is where you have to wash the dishes.
  -- #debian-devel, Freenode, 2004-09-22

Re: [PATCH] nbd: show nbd client pid in sysfs

[Pavel Machek]

On Mon 2006-12-11 00:18:01, Wouter Verhelst wrote:
> On Sun, Dec 10, 2006 at 08:58:19PM +0100, Pavel Machek wrote:
> > Hi!
> 
> Hi
> 
> > > > > This simple patch allows nbd to expose the nbd-client 
> > > > > daemon's PID in /sys/block/nbd<x>/pid. This is helpful 
> > > > > for tracking connection status of a device and for 
> > > > > determining which nbd devices are currently in use.
> > > > 
> > > > Actually is it needed at all? Perhaps nbd clients should be modified
> > > > to put nbdX in their process nam?
> > > 
> > > I don't think that's the right approach; only the kernel can guarantee
> > > that a given process is actually managing a given nbd device (I could
> > > have some rogue process running around announcing that it's managing
> > > nbd2, and then what?)
> > 
> > I'd say "do not run rogue processes as root" :-).
> > 
> > nbd-client should run as root -- I do not think interface was
> > carefully audited to run it as a user -- so rogue process should not
> > really be a problem.
> 
> IOW, you're suggesting I walk the process list from userspace to find a
> process for which a) it claims it's running for a given nbd device, and
> b) I can verify that it actually has the permissions to do so. That's a
> whole lot of code in comparison to
> 
> f=open("/sys/block/nbd2/pid", O_RDONLY);
> read(f,buf,len);
> 
> I think I very much prefer the above two lines, not only for simplicity.
> 
> Also, your suggestion relies on users /only/ using the official
> nbd-client, and is fragile in cases where that assumption is false
> (i.e., it's susceptible to false negatives). The suggested patch does
> not have that problem.

I do not think finding out "which pid is controlling nbd#2" is _that_
important.

We have /var/lock files for ttyS*, I do not see reason why nbd should
be different. Actually you could copy /var/lock approach.

And BTW that /sys/ thingie is racy by design (as is the log file). By
the time you try to do anything with that PID, process may be gone.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html