Re: [Nbd] [w@...112...: Re: nbd-client started at boot (for root device) is not persistent]

[Victor <vixtor@...17...>]

Here is an updated patch version, better matching your suggestions with a few comments:

a) I create the device with mknod, but i create it in /dev, not /tmp, because run-init deleted /tmp anyway. So if I have to re-create a directory, I have to choose between recreating /dev and recreating /tmp. So why not recreate /tmp and leave the nbddev string as it is, unchanged and less confusing? And if /dev/nbd0 was deleted for some reason but /dev is still udev-mounted, i can create it there manually anyway and it will be gone at reboot, which is ok. The other option (which avoided any directory creation) was to create the device directly in / but for some reason I find this too ugly.

b) your current solution (checking for the real pid of the kernel process) seems to me more elegant than just waiting a second. So i have made the check_conn function to test for /sys existence, and if it is missing, it will mount it, read what it needs and umount it back. This way, /sys will not remain mounted (and this mount/read/umount happens only once at reconnect, so it's not a big harm to do it).

On Sun, Nov 27, 2016 at 6:23 PM, Wouter Verhelst <w@...112...> wrote:

Hi Victor,

On Fri, Nov 25, 2016 at 01:23:04PM +0200, Victor wrote:
> I have found an idea that seems to work fine - at least for me: if the process
> cannot find /dev and /sys anymore, it means it was started from initramfs. So i
> recreate /sys and /dev and mount sysfs and devtmpfs again. The VFS tree of
> nbd-client will get a duplicate mount of /dev and /sys but i don't think
> there's any harm in that.

I think there is; it can interfere with proper shutdown, depending on
the init system in use.

A better way is to:

- Check if /sys exists; if not, wait a second rather than checking for a
  pid file (perhaps best to ensure we don't fork() until the connection
  has been made then, though)
- Check if /dev exists; if not, run mknod(2) on a name in /tmp (or some
  such) with the correct attributes, open it, and unlink the file again

This doesn't leave any traces around anymore, and should allow
nbd-client to do what it needs to do.

I'll look into making the necessary changes when next I have time to
work on NBD.

--
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

--- nbd-3.13/nbd-client.c	2016-01-02 21:34:16.000000000 +0200
+++ nbd-3.13-modified/nbd-client.c	2016-11-28 08:19:14.047695772 +0200
@@ -25,6 +25,7 @@
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <sys/types.h>
+#include <sys/stat.h>
 #include <unistd.h>
 #include <netinet/tcp.h>
 #include <netinet/in.h>
@@ -53,11 +54,34 @@
 
 #define NBDC_DO_LIST 1
 
+void check_conn_cleanup(char *devname, int fd, int sysfs, int sysdir) {
+	if(fd>0) {
+		close(fd);
+	}
+	if(sysfs) {
+		if(umount("/sys") == 0) {
+			fprintf(stderr, "sysfs unmounted\n");
+		} else {
+			err("failed to umount sysfs");
+		}
+	}
+	if(sysdir) {
+		if(rmdir("/sys") == 0) {
+			fprintf(stderr, "/sys deleted\n");
+		} else {
+			err("failed to delete /sys");
+		}
+	}
+}
+
 int check_conn(char* devname, int do_print) {
 	char buf[256];
 	char* p;
 	int fd;
 	int len;
+	struct stat sb;
+	int sysdir = 0;
+	int sysfs = 0;
 
 	if( (p=strrchr(devname, '/')) ) {
 		devname=p+1;
@@ -66,8 +90,43 @@
 		/* We can't do checks on partitions. */
 		*p='\0';
 	}
+
+	if (stat("/sys/block", &sb) == 0 && S_ISDIR(sb.st_mode)) {
+		/* we have a valid sysfs */
+		fprintf(stderr, "%s: we have a valid sysfs, that's good\n", devname);
+	} else {
+		fprintf(stderr, "%s: /sys/block is missing, we may need to mount sysfs\n", devname);
+		if(stat("/sys", &sb) == 0) { 
+			if(S_ISDIR(sb.st_mode)) {
+				fprintf(stderr, "%s: /sys exists\n", devname);
+				if(mount("sysfs", "/sys", "sysfs", MS_NOSUID|MS_NODEV|MS_NOEXEC|MS_RELATIME, "") < 0) {
+					err("Failed to mount sysfs");
+				} else {
+					fprintf(stderr, "%s: sysfs mounted\n", devname);
+					sysfs = 1;
+				}
+			} else {
+				err("/sys exists, but it's not a directory");
+			}
+		} else {
+			if(mkdir("/sys", 0555) < 0) {
+				err("Cannot re-create /sys");
+			} else {
+				fprintf(stderr, "%s: /sys created\n", devname);
+				sysdir = 1;
+				if(mount("sysfs", "/sys", "sysfs", MS_NOSUID|MS_NODEV|MS_NOEXEC|MS_RELATIME, "") < 0) {
+					err("Failed to mount sysfs");
+				} else {
+					fprintf(stderr, "%s: sysfs mounted\n", devname);
+					sysfs = 1;
+				}
+			}
+		}
+	}
+
 	snprintf(buf, 256, "/sys/block/%s/pid", devname);
 	if((fd=open(buf, O_RDONLY))<0) {
+		check_conn_cleanup(devname, fd, sysfs, sysdir);
 		if(errno==ENOENT) {
 			return 1;
 		} else {
@@ -76,11 +135,13 @@
 	}
 	len=read(fd, buf, 256);
 	if(len < 0) {
+		check_conn_cleanup(devname, fd, sysfs, sysdir);
 		perror("could not read from server");
 		return 2;
 	}
 	buf[(len < 256) ? len : 255]='\0';
 	if(do_print) printf("%s\n", buf);
+	check_conn_cleanup(devname, fd, sysfs, sysdir);
 	return 0;
 }
 
@@ -550,12 +611,74 @@
 	printf("done\n");
 }
 
+int create_blkdev(char *nbddev, int flags, unsigned int *major, unsigned int *minor) {
+	struct stat sb;
+	if(*major==43 && *minor>=0) {
+		fprintf(stderr, "%s: trying to create as block(%d,%d)\n", nbddev, *major, *minor);
+
+		if(stat("/dev", &sb) == 0) {
+			if(S_ISDIR(sb.st_mode)) {
+				fprintf(stderr, "%s: /dev exists\n", nbddev);
+			} else {
+				fprintf(stderr, "%s: /dev exists, but it's not a directory", nbddev);
+				return -1;
+			}
+		} else {
+                        if(mkdir("/dev", 0755) < 0) {
+				fprintf(stderr, "%s: failed to create /dev", nbddev);
+				return -1;
+			} else {
+				fprintf(stderr, "%s: /dev created\n", nbddev);
+			}
+		}
+
+		if(mknod(nbddev, S_IFBLK, makedev(*major, *minor)) == 0) {
+			fprintf(stderr, "%s: created\n", nbddev);
+			return open(nbddev, flags);
+		} else {
+			fprintf(stderr, "%s: failed to create device inode\n", nbddev);
+			return -1;
+		}
+	} else {
+		fprintf(stderr, "%s: major/minor unknown, cannot create\n", nbddev);
+		return -1;
+	}
+}
+
+int open_dev(char *nbddev, int flags, unsigned int *major, unsigned int *minor) {
+	int nbd;
+	struct stat sb;
+
+	if(stat(nbddev, &sb) == 0) {
+		fprintf(stderr, "%s: device inode exists\n", nbddev);			
+		if (S_ISBLK(sb.st_mode)) {
+			fprintf(stderr, "%s: is a block device\n", nbddev);
+			if(major(sb.st_rdev) == 43) {
+				*major = 43;
+				*minor = minor(sb.st_rdev);
+				fprintf(stderr, "%s: has the correct major, saving minor %d\n", nbddev, *minor);
+				return open(nbddev, flags);
+			} else {
+				fprintf(stderr, "%s: wrong major\n", nbddev);
+				return -1;
+			}
+		} else {
+			fprintf(stderr, "%s: not a block device\n", nbddev);
+			return -1;
+		}
+	} else {
+		fprintf(stderr, "%s: does not exit\n", nbddev);
+		return create_blkdev(nbddev, flags, major, minor);
+	}
+}
+
 int main(int argc, char *argv[]) {
 	char* port=NBD_DEFAULT_PORT;
 	int sock, nbd;
 	int blocksize=1024;
 	char *hostname=NULL;
 	char *nbddev=NULL;
+	unsigned int major=-1, minor=-1;
 	int swap=0;
 	int cont=0;
 	int timeout=0;
@@ -716,7 +839,7 @@
 
 	negotiate(sock, &size64, &flags, name, needed_flags, cflags, opts);
 
-	nbd = open(nbddev, O_RDWR);
+	nbd = open_dev(nbddev, O_RDWR, &major, &minor);
 	if (nbd < 0)
 	  err("Cannot open NBD: %m\nPlease ensure the 'nbd' module is loaded.");
 
@@ -799,7 +922,7 @@
 							break;
 						sleep (1);
 					}
-					nbd = open(nbddev, O_RDWR);
+					nbd = open_dev(nbddev, O_RDWR, &major, &minor);
 					if (nbd < 0)
 						err("Cannot open NBD: %m");
 					negotiate(sock, &new_size, &new_flags, name, needed_flags, cflags, opts);