[PATCH] mount.cifs: make return codes match the return codes for /bin/mount (try #2)

[Jeff Layton-2]

The manpage for /bin/mount specifies that the return code should be a
positive integer (actually, it's a bitfield). Clean up the return
codes from mount.cifs to make them match the expected return values
from /bin/mount. This necessary for proper integration with autofs.

This is the second attempt at this patch. The main difference here
is that this one uses #define'd constants for the exit codes. I
also changed a few places to return EX_SYSERR rather than EX_USAGE
since it looked like a more appropriate error.

Signed-off-by: Jeff Layton <jlayton@...>
---
 source3/client/mount.cifs.c |   80 ++++++++++++++++++++++++-------------------
 1 files changed, 45 insertions(+), 35 deletions(-)

diff --git a/source3/client/mount.cifs.c b/source3/client/mount.cifs.c
index b7a76c6..9d357cd 100644
--- a/source3/client/mount.cifs.c
+++ b/source3/client/mount.cifs.c
@@ -79,6 +79,15 @@
 #define MOUNT_PASSWD_SIZE 64
 #define DOMAIN_SIZE 64
 
+/* exit status - bits below are ORed */
+#define EX_USAGE        1       /* incorrect invocation or permission */
+#define EX_SYSERR       2       /* out of memory, cannot fork, ... */
+#define EX_SOFTWARE     4       /* internal mount bug or wrong version */
+#define EX_USER         8       /* user interrupt */
+#define EX_FILEIO      16       /* problems writing, locking, ... mtab/fstab */
+#define EX_FAIL        32       /* mount failure */
+#define EX_SOMEOK      64       /* some mount succeeded */
+
 const char *thisprogram;
 int verboseflag = 0;
 static int got_password = 0;
@@ -174,7 +183,7 @@ static void mount_cifs_usage(void)
  printf("\n\t%s -V\n",thisprogram);
 
  SAFE_FREE(mountpassword);
- exit(1);
+ exit(EX_USAGE);
 }
 
 /* caller frees username if necessary */
@@ -233,7 +242,7 @@ static int open_cred_file(char * file_name)
  if(length > 4086) {
  printf("mount.cifs failed due to malformed username in credentials file");
  memset(line_buf,0,4096);
- exit(1);
+ exit(EX_USAGE);
  } else {
  got_user = 1;
  user_name = (char *)calloc(1 + length,1);
@@ -257,7 +266,7 @@ static int open_cred_file(char * file_name)
  if(length > MOUNT_PASSWD_SIZE) {
  printf("mount.cifs failed: password in credentials file too long\n");
  memset(line_buf,0, 4096);
- exit(1);
+ exit(EX_USAGE);
  } else {
  if(mountpassword == NULL) {
  mountpassword = (char *)calloc(MOUNT_PASSWD_SIZE+1,1);
@@ -285,7 +294,7 @@ static int open_cred_file(char * file_name)
                                 }
                                 if(length > DOMAIN_SIZE) {
                                         printf("mount.cifs failed: domain in credentials file too long\n");
-                                        exit(1);
+                                        exit(EX_USAGE);
                                 } else {
                                         if(domain_name == NULL) {
                                                 domain_name = (char *)calloc(DOMAIN_SIZE+1,1);
@@ -318,7 +327,7 @@ static int get_password_from_file(int file_descript, char * filename)
 
  if (mountpassword == NULL) {
  printf("malloc failed\n");
- exit(1);
+ exit(EX_SYSERR);
  }
 
  if(filename != NULL) {
@@ -326,7 +335,7 @@ static int get_password_from_file(int file_descript, char * filename)
  if(file_descript < 0) {
  printf("mount.cifs failed. %s attempting to open password file %s\n",
    strerror(errno),filename);
- exit(1);
+ exit(EX_SYSERR);
  }
  }
  /* else file already open and fd provided */
@@ -337,7 +346,7 @@ static int get_password_from_file(int file_descript, char * filename)
  printf("mount.cifs failed. Error %s reading password file\n",strerror(errno));
  if(filename != NULL)
  close(file_descript);
- exit(1);
+ exit(EX_SYSERR);
  } else if(rc == 0) {
  if(mountpassword[0] == 0) {
  if(verboseflag)
@@ -563,7 +572,7 @@ static int parse_options(char ** optionsp, int * filesys_flags)
 
  if (!(pw = getpwnam(value))) {
  printf("bad user name \"%s\"\n", value);
- exit(1);
+ exit(EX_USAGE);
  }
  snprintf(user, sizeof(user), "%u", pw->pw_uid);
  } else {
@@ -579,7 +588,7 @@ static int parse_options(char ** optionsp, int * filesys_flags)
 
  if (!(gr = getgrnam(value))) {
  printf("bad group name \"%s\"\n", value);
- exit(1);
+ exit(EX_USAGE);
  }
  snprintf(group, sizeof(group), "%u", gr->gr_gid);
  } else {
@@ -674,7 +683,7 @@ static int parse_options(char ** optionsp, int * filesys_flags)
  out = (char *)realloc(out, out_len + word_len + 2);
  if (out == NULL) {
  perror("malloc");
- exit(1);
+ exit(EX_SYSERR);
  }
 
  if (out_len) {
@@ -699,7 +708,7 @@ nocopy:
  out = (char *)realloc(out, out_len + word_len + 6);
  if (out == NULL) {
  perror("malloc");
- exit(1);
+ exit(EX_SYSERR);
  }
 
  if (out_len) {
@@ -715,7 +724,7 @@ nocopy:
  out = (char *)realloc(out, out_len + 1 + word_len + 6);
  if (out == NULL) {
  perror("malloc");
- exit(1);
+ exit(EX_SYSERR);
  }
 
  if (out_len) {
@@ -1050,7 +1059,7 @@ int main(int argc, char ** argv)
  thisprogram = argv[0];
  } else {
  mount_cifs_usage();
- exit(1);
+ exit(EX_USAGE);
  }
 
  if(thisprogram == NULL)
@@ -1067,12 +1076,12 @@ int main(int argc, char ** argv)
  share_name = strndup(argv[1], MAX_UNC_LEN);
  if (share_name == NULL) {
  fprintf(stderr, "%s: %s", argv[0], strerror(ENOMEM));
- exit(1);
+ exit(EX_SYSERR);
  }
  mountpoint = argv[2];
  } else {
  mount_cifs_usage();
- exit(1);
+ exit(EX_USAGE);
  }
 
  /* add sharename in opts string as unc= parm */
@@ -1094,7 +1103,7 @@ int main(int argc, char ** argv)
  case '?':
  case 'h': /* help */
  mount_cifs_usage ();
- exit(1);
+ exit(EX_USAGE);
  case 'n':
     ++nomtab;
     break;
@@ -1148,14 +1157,14 @@ int main(int argc, char ** argv)
  uid = strtoul(optarg, &ep, 10);
  if (*ep) {
  printf("bad uid value \"%s\"\n", optarg);
- exit(1);
+ exit(EX_USAGE);
  }
  } else {
  struct passwd *pw;
 
  if (!(pw = getpwnam(optarg))) {
  printf("bad user name \"%s\"\n", optarg);
- exit(1);
+ exit(EX_USAGE);
  }
  uid = pw->pw_uid;
  endpwent();
@@ -1168,14 +1177,14 @@ int main(int argc, char ** argv)
  gid = strtoul(optarg, &ep, 10);
  if (*ep) {
  printf("bad gid value \"%s\"\n", optarg);
- exit(1);
+ exit(EX_USAGE);
  }
  } else {
  struct group *gr;
 
  if (!(gr = getgrnam(optarg))) {
  printf("bad user name \"%s\"\n", optarg);
- exit(1);
+ exit(EX_USAGE);
  }
  gid = gr->gr_gid;
  endpwent();
@@ -1205,13 +1214,13 @@ int main(int argc, char ** argv)
  default:
  printf("unknown mount option %c\n",c);
  mount_cifs_usage();
- exit(1);
+ exit(EX_USAGE);
  }
  }
 
  if((argc < 3) || (dev_name == NULL) || (mountpoint == NULL)) {
  mount_cifs_usage();
- exit(1);
+ exit(EX_USAGE);
  }
 
  if (getenv("PASSWD")) {
@@ -1228,13 +1237,13 @@ int main(int argc, char ** argv)
  }
 
         if (orgoptions && parse_options(&orgoptions, &flags)) {
-                rc = -1;
+                rc = EX_USAGE;
  goto mount_exit;
  }
  ipaddr = parse_server(&share_name);
  if((ipaddr == NULL) && (got_ip == 0)) {
  printf("No ip address specified and hostname not found\n");
- rc = -1;
+ rc = EX_USAGE;
  goto mount_exit;
  }
 
@@ -1249,19 +1258,19 @@ int main(int argc, char ** argv)
  }
  if(chdir(mountpoint)) {
  printf("mount error: can not change directory into mount target %s\n",mountpoint);
- rc = -1;
+ rc = EX_USAGE;
  goto mount_exit;
  }
 
  if(stat (".", &statbuf)) {
  printf("mount error: mount point %s does not exist\n",mountpoint);
- rc = -1;
+ rc = EX_USAGE;
  goto mount_exit;
  }
 
  if (S_ISDIR(statbuf.st_mode) == 0) {
  printf("mount error: mount point %s is not a directory\n",mountpoint);
- rc = -1;
+ rc = EX_USAGE;
  goto mount_exit;
  }
 
@@ -1274,7 +1283,7 @@ int main(int argc, char ** argv)
 #endif
  } else {
  printf("mount error: permission denied or not superuser and mount.cifs not installed SUID\n");
- return -1;
+ return EX_USAGE;
  }
  }
 
@@ -1289,7 +1298,7 @@ int main(int argc, char ** argv)
  mountpassword = (char *)calloc(MOUNT_PASSWD_SIZE+1,1);
  if (!tmp_pass || !mountpassword) {
  printf("Password not entered, exiting\n");
- return -1;
+ return EX_USAGE;
  }
  strlcpy(mountpassword, tmp_pass, MOUNT_PASSWD_SIZE+1);
  got_password = 1;
@@ -1307,7 +1316,7 @@ mount_retry:
  else {
  printf("No server share name specified\n");
  printf("\nMounting the DFS root for server not implemented yet\n");
-                exit(1);
+                exit(EX_USAGE);
  }
  if(user_name)
  optlen += strlen(user_name) + 6;
@@ -1321,7 +1330,7 @@ mount_retry:
 
  if(options == NULL) {
  printf("Could not allocate memory for mount options\n");
- return -1;
+ return EX_SYSERR;
  }
 
  options[0] = 0;
@@ -1400,8 +1409,7 @@ mount_retry:
  printf("mount error %d = %s\n",errno,strerror(errno));
  }
  printf("Refer to the mount.cifs(8) manual page (e.g.man mount.cifs)\n");
- rc = -1;
- goto mount_exit;
+ rc = EX_FAIL;
  } else {
  pmntfile = setmntent(MOUNTED, "a+");
  if(pmntfile) {
@@ -1439,11 +1447,13 @@ mount_retry:
  rc = addmntent(pmntfile,&mountent);
  endmntent(pmntfile);
  SAFE_FREE(mountent.mnt_opts);
+ if (rc)
+ rc = EX_FILEIO;
  } else {
-    printf("could not update mount table\n");
+ printf("could not update mount table\n");
+ rc = EX_FILEIO;
  }
  }
- rc = 0;
 mount_exit:
  if(mountpassword) {
  int len = strlen(mountpassword);
--
1.5.5.1

[Jeff Moyer]

Jeff Layton <jlayton@...> writes:

> The manpage for /bin/mount specifies that the return code should be a
> positive integer (actually, it's a bitfield). Clean up the return
> codes from mount.cifs to make them match the expected return values
> from /bin/mount. This necessary for proper integration with autofs.
>
> This is the second attempt at this patch. The main difference here
> is that this one uses #define'd constants for the exit codes. I
> also changed a few places to return EX_SYSERR rather than EX_USAGE
> since it looked like a more appropriate error.
>
> Signed-off-by: Jeff Layton <jlayton@...>

Well, I don't like the mixed exit()s and return's, but that was there
before.  There's also the question of whether you really want to return
the same error code for all of the below cases:

        if(mount(dev_name, mountpoint, "cifs", flags, options)) {
                case 0:
                case ENODEV:
                case ENXIO:
                default:

But again, that was already there.  If folks find those things
palatable, then I'm okay with this.  I guess it's worth noting that I am
somewhat familiar with the mount code, and I believe this is exactly
what it expects, since it just returns the exit status of the
fs-specific mount command in the failure case.

Acked-by: Jeff Moyer <jmoyer@...>

Cheers,

Jeff

[Jeff Layton-2]

On Wed, 08 Oct 2008 14:26:07 -0400
Jeff Moyer <jmoyer@...> wrote:

> Jeff Layton <jlayton@...> writes:
>
> > The manpage for /bin/mount specifies that the return code should be a
> > positive integer (actually, it's a bitfield). Clean up the return
> > codes from mount.cifs to make them match the expected return values
> > from /bin/mount. This necessary for proper integration with autofs.
> >
> > This is the second attempt at this patch. The main difference here
> > is that this one uses #define'd constants for the exit codes. I
> > also changed a few places to return EX_SYSERR rather than EX_USAGE
> > since it looked like a more appropriate error.
> >
> > Signed-off-by: Jeff Layton <jlayton@...>
>
> Well, I don't like the mixed exit()s and return's, but that was there
> before.  There's also the question of whether you really want to return
> the same error code for all of the below cases:
>
>         if(mount(dev_name, mountpoint, "cifs", flags, options)) {
>                 case 0:
>                 case ENODEV:
>                 case ENXIO:
>                 default:
>
> But again, that was already there.  If folks find those things
> palatable, then I'm okay with this.  I guess it's worth noting that I am
> somewhat familiar with the mount code, and I believe this is exactly
> what it expects, since it just returns the exit status of the
> fs-specific mount command in the failure case.
>
> Acked-by: Jeff Moyer <jmoyer@...>
>

Thanks Jeff,

   I've got a new patch that replaces the "return -1" calls with
exit(EX_USAGE) for most of these cases, so we should be able to make
that more consistent. I'll plan to commit that one tomorrow unless
anyone has objections.

For the switch statement, I'd rather hold off on making changes there.
I'm working on some cleanup to the cifs_mount() kernel code now as
well. I'd like to wait until that's closer to complete and then
reevaluate the switch as a whole. There may be other cases we need to
handle, and I'm not convinced that the printf's are currently always
correct for the cases that we have there.

--
Jeff Layton <jlayton@...>

[Jeff Layton-2]

On Wed, 8 Oct 2008 14:58:02 -0400
Jeff Layton <jlayton@...> wrote:

> On Wed, 08 Oct 2008 14:26:07 -0400
> Jeff Moyer <jmoyer@...> wrote:
>
> > Jeff Layton <jlayton@...> writes:
> >
> > > The manpage for /bin/mount specifies that the return code should be a
> > > positive integer (actually, it's a bitfield). Clean up the return
> > > codes from mount.cifs to make them match the expected return values
> > > from /bin/mount. This necessary for proper integration with autofs.
> > >
> > > This is the second attempt at this patch. The main difference here
> > > is that this one uses #define'd constants for the exit codes. I
> > > also changed a few places to return EX_SYSERR rather than EX_USAGE
> > > since it looked like a more appropriate error.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@...>
> >
> > Well, I don't like the mixed exit()s and return's, but that was there
> > before.  There's also the question of whether you really want to return
> > the same error code for all of the below cases:
> >
> >         if(mount(dev_name, mountpoint, "cifs", flags, options)) {
> >                 case 0:
> >                 case ENODEV:
> >                 case ENXIO:
> >                 default:
> >
> > But again, that was already there.  If folks find those things
> > palatable, then I'm okay with this.  I guess it's worth noting that I am
> > somewhat familiar with the mount code, and I believe this is exactly
> > what it expects, since it just returns the exit status of the
> > fs-specific mount command in the failure case.
> >
> > Acked-by: Jeff Moyer <jmoyer@...>
> >
>
> Thanks Jeff,
>
>    I've got a new patch that replaces the "return -1" calls with
> exit(EX_USAGE) for most of these cases, so we should be able to make
> that more consistent. I'll plan to commit that one tomorrow unless
> anyone has objections.
>
> For the switch statement, I'd rather hold off on making changes there.
> I'm working on some cleanup to the cifs_mount() kernel code now as
> well. I'd like to wait until that's closer to complete and then
> reevaluate the switch as a whole. There may be other cases we need to
> handle, and I'm not convinced that the printf's are currently always
> correct for the cases that we have there.
>

Patch pushed to samba master, and v3-x-test branches.

--
Jeff Layton <jlayton@...>