Re: Alternative patch for parsing credentials file in mount.cifs.c

On Tue, 20 Apr 2010 09:55:57 -0400
Scott Lovenberg <[email protected]> wrote:

> Just for kicks and giggles I moved the parsing for the samba credentials 
> file to its own function.  I actually really like this approach a bit 
> more than how it was.  The flow is a bit better and there is a bit less 
> redundant code.  What do you think?
> 
> (/OT - I just found out the joy of branch merging in git.. I think I 
> might be in love with it.  It flawlessly merged three commits in two 
> branches and gave me a diff against the master./)
> 
> 
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 98e5665..4ef3a67 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -121,6 +121,14 @@
>    */
>   #define CIFS_SETUID_FLAGS (MS_NOSUID|MS_NODEV)
> 
> +/*
> + * Values for parsing a credentials file.
> + */
> +#define CRED_UNPARSABLE 0
> +#define CRED_USER 1
> +#define CRED_PASS 2
> +#define CRED_DOM 4
> +
>   /* struct for holding parsed mount info for use by privleged process */
>   struct parsed_mount_info {
>       unsigned long flags;
> @@ -521,6 +529,34 @@ static void null_terminate_endl(char* source)
>           *newline = '\0';
>   }
> 
> +/*
> + * Parse a line from the credentials file.  Points 'target' to the 
> first character
> + * after '=' on 'line' and returns the value type (CRED_) of the line.
> + * Returns CRED_UNPARSABLE on failure or if either parameter is NULL.
> + */
> +static int parse_cred_line(char* line, char* target)
> +{
> +    if (line == NULL || target == NULL)
> +        goto parsing_err;
> +
> +    /* position target at first char of value */
> +    target = strchr(line, '=');
> +    if (!target)
> +        goto parsing_err;
> +    target++;
> +
> +    if (strncasecmp("user", line, 4) == 0)
> +        return CRED_USER;
> +    if (strncasecmp("pass", line, 4) == 0)
> +        return CRED_PASS;
> +    if (strncasecmp("dom", line, 3) == 0)
> +        return CRED_DOM;
> +
> +    /* if we're here, the line or target wasn't sane */
> +    parsing_err:
> +        return CRED_UNPARSABLE;
> +}
> +
>   static int open_cred_file(char *file_name,
>                 struct parsed_mount_info *parsed_info)
>   {
> @@ -559,7 +595,7 @@ static int open_cred_file(char *file_name,
>           return EX_SYSERR;
>       }
> 
> -    /* parse line from credential file*/
> +    /* parse line from credential file */
>       while (fgets(line_buf, line_buf_size, fs)) {
>           /* eat leading white space */
>           for (i = 0; i < line_buf_size - min_non_white + 1; i++) {
> @@ -568,48 +604,40 @@ static int open_cred_file(char *file_name,
>           }
>           null_terminate_endl(line_buf);
> 
> -        /* parse user */
> -        if (strncasecmp("user", line_buf + i, 4) == 0) {
> -            temp_val = strchr(line_buf + i, '=');
> -            if (!temp_val)
> -                continue;
> -            /* go past equals sign */
> -            temp_val++;
> -            parse_username(temp_val, parsed_info);
> -            memset(line_buf, 0, line_buf_size);
> -        }
> -
> -        /* parse password */
> -        else if (strncasecmp("pass", line_buf + i, 4) == 0) {
> -            temp_val = strchr(line_buf + i, '=');
> -            if (!temp_val)
> -                continue;
> -                        /* go past equals sign */
> -            temp_val++;
> -            i = set_password(parsed_info, temp_val);
> -            /* zero out password from buffer */
> -            memset(line_buf, 0, line_buf_size);
> -            if (i)
> -                return i;
> -        }
> -
> -        /* parse domain */
> -        else if (strncasecmp("dom", line_buf + i, 3) == 0) {
> -            temp_val = strchr(line_buf + i, '=');
> -            if (!temp_val)
> -                continue;
> -            /* go past equals sign */
> -            temp_val++;
> -            if (parsed_info->verboseflag)
> -                fprintf(stderr, "\nDomain %s\n", temp_val);
> -            strlcpy(parsed_info->domain, temp_val,
> -                sizeof(parsed_info->domain));
> -            }
> +        switch(parse_cred_line(line_buf, temp_val)) {
> +            /* user */
> +            case CRED_USER:
> +                parse_username(temp_val, parsed_info);
> +            break;
> +
> +            /* password */
> +            case CRED_PASS:
> +                i = set_password(parsed_info, temp_val);
> +                if (i)
> +                    goto return_i;
> +            break;
> +
> +            /* domain */
> +            case CRED_DOM:
> +                if (parsed_info->verboseflag)
> +                    fprintf(stderr, "\nDomain %s\n",
> +                        temp_val);
> +                strlcpy(parsed_info->domain, temp_val,
> +                    sizeof(parsed_info->domain));
> +            break;
>           }
>       }
> -    fclose(fs);
> -    SAFE_FREE(line_buf);
> -    return 0;
> +
> +    /* no errors */
> +    i = 0;
> +
> +    /* jump point for errors */
> +    return_i:
> +        fclose(fs);
> +        memset(line_buf, 0, line_buf_size);
> +        SAFE_FREE(line_buf);
> +        SAFE_FREE(temp_val);
> +        return i;
>   }
> 
>   static int
> 

This patch is malformed. Looks like you pasted it into your mailer and
it line-wrapped it? The best way to do this is to commit the patch to
your repo (via git-commit), and then use git-format-patch to generate a
patch file. Then use git-send-email to send it. That's generally how I
send patches and it helps prevent the problem you're hitting.

-- 
Jeff Layton <[email protected]>