From: "Chakravarthi P" <pchakravarthi@novell.com>
Subject: Re: [PATCH 0 / 1] Move NFS mount code from util-linux
	to	nfs-utils - take2
Date: Thu, 15 Jun 2006 05:10:24 -0600
Message-ID: <44918BDD.2C84.006B.0@novell.com>
References: <448DF279.8090005@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Cc: nfs@lists.sourceforge.net, Steve Dickson <SteveD@redhat.com>
To: "Amit Gud" <agud@redhat.com>, "Neil Brown" <neilb@suse.de>
In-Reply-To: <448DF279.8090005@redhat.com>
Sender: nfs-bounces@lists.sourceforge.net
Errors-To: nfs-bounces@lists.sourceforge.net


in mount.c :

+void mount_usage()
+{
+	printf("usage: %s remotetarget dir [-rvVwfnh] [-t version] [-o
nfsoptions]\n", progname);
+	printf("options:\n\t-r\t\tMount file system readonly\n");
+	printf("\t-v\t\tVerbose\n");
+	printf("\t-V\t\tPrint version\n");
+	printf("\t-w\t\tMount file system read-write\n");

1. isn't it generally a good practice to have usage message over stderr
?
so the user doesn't miss it. even if he chances to redirect the output
...
just a suggestion.


+			break;
+		case 't':
+			nfs_mount_vers = (strncmp(optarg, "nfs4", 4)) ?
0 : 4;
+			break;

2.  i dont think a -t option makes sense for mount.nfs  
     mount.nfs itself says it is a nfs file system
     you can pick up the version from -o nfsvers=4  to cater to nfs
version
     -t is by legacy meant for specifying the filesystem type.
      
      i also don't see why bind and other options need to be there for
mount.nfs


+
+int add_mtab(char *fsname, char *mount_point, char *fstype, int flags,
char *opts, int freq, int passno)
+{

3. felt that there can be just be one function called mtab_update that
can be kept in 
    the common utility source files you had so that both umount and
mount code can use it.

in nfs4mount.c
+#if defined(VAR_LOCK_DIR)
+#define DEFAULT_DIR VAR_LOCK_DIR
+#else
+#define DEFAULT_DIR "/var/lock/subsys"
+#endif
+
+extern int verbose;
+
+char *IDMAPLCK = DEFAULT_DIR "/rpcidmapd";
+#define idmapd_check() do { \
+	if (access(IDMAPLCK, F_OK)) { \
+		printf(_("Warning: rpc.idmapd appears not to be
running.\n" \
+			"         All uids will be mapped to the nobody
uid.\n")); \
+	} \
+} while(0);
+
+char *GSSDLCK = DEFAULT_DIR "/rpcgssd";
+#define gssd_check() do { \
+		if (access(GSSDLCK, F_OK)) { \
+			printf(_("Warning: rpc.gssd appears not to be
running.\n")); \
+		} \
+} while(0); 


4. very nice thing to do ... warn the user if idmapd and gssd are not
running.
    But then idmapd on suse doesn't seem to be writing into
/var/lock/subsys ... 
    Is /var/lock/subsys/  the standard way for daemons to show their
existence. ?
    or is creation of  /var/run/<daemon_name>  a standard ?
    then idmapd scripts might need some change .. or else ..
    code needs some modification here to be able to work fine in all
distros ...

in nfsumount.c :

+	spec = argv[1];
+
+	argv += 1;
+	argc -= 1;
+
+	while ((c = getopt_long (argc, argv, "fvnrlh",
+				umount_longopts, NULL)) != -1) {

 it'll be nice if you include -a option and have all nfs or nfs4 mounts
umounted on calling
 umount.nfs -a 
 this is should be easy enough with an appropriate mtab utility
function.


 thanks
 chax.


P. S. Chakravarthi
Senior Software Engineer
pchakravarthi@novell.com
Phone: 25731856 Extn: 3198

Novell Inc.
Software for the Open Enterprise


_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs