| Message ID | 1366075789-23137-1-git-send-email-scott.lovenberg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 15, 2013 at 9:29 PM, <scott.lovenberg@gmail.com> wrote: > From: Scott Lovenberg <scott.lovenberg@gmail.com> > > This is a respin of the last patch that returns EX_USAGE rather than OPT_ERROR as per Jeff's suggestion. > > Signed-off-by: Scott Lovenberg <scott.lovenberg@gmail.com> For future reference, is there a way to get git send-email to attach this to another email thread? I tried to use --in-reply-to=1365963558-20082-1-git-send-email-scott.lovenberg@gmail.com, but that seems to not be correct and my Google-fu seems to be weak today. -- Peace and Blessings, -Scott. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 15, 2013 at 9:29 PM, <scott.lovenberg@gmail.com> wrote: > > From: Scott Lovenberg <scott.lovenberg@gmail.com> > > This is a respin of the last patch that returns EX_USAGE rather than OPT_ERROR as per Jeff's suggestion. > > Signed-off-by: Scott Lovenberg <scott.lovenberg@gmail.com> > --- > AUTHORS | 1 + > mount.cifs.c | 62 ++++++++++++++++++++++++++++++++++++++++++---------------- > 2 files changed, 46 insertions(+), 17 deletions(-) > > diff --git a/AUTHORS b/AUTHORS > index 2807079..081c2fe 100644 > --- a/AUTHORS > +++ b/AUTHORS > @@ -5,5 +5,6 @@ Shirish Pargaonkar <shirishpargaonkar@gmail.com> > Suresh Jayaraman <sjayaraman@suse.de> > Pavel Shilovsky <piastry@etersoft.ru> > Igor Druzhinin <jaxbrigs@gmail.com> > +Scott Lovenberg <scott.lovenberg@gmail.com> > > ...and others. > diff --git a/mount.cifs.c b/mount.cifs.c > index 3b2b89e..561517b 100644 > --- a/mount.cifs.c > +++ b/mount.cifs.c > @@ -751,6 +751,47 @@ static int parse_opt_token(const char *token) > return OPT_ERROR; > } > > +/* > + Extract an ipv4 or ipv6 address and put it into parsed_info. > + Returns 0 on success. > +*/ > +static int parse_ip(char *value, struct parsed_mount_info *parsed_info) > +{ > + char *closingBracket = NULL; > + > + if (!value || !*value) { > + fprintf(stderr, "target ip address argument missing\n"); > + return EX_USAGE; > + } > + > + /* check for ipv6 in brackets */ > + if (value[0] == '[') { > + value++; > + /* temporarily null the closing bracket */ > + closingBracket = strchr(value, ']'); > + if (closingBracket) > + *closingBracket = '\0'; > + } > + > + /* check length of address */ > + if (strnlen(value, MAX_ADDRESS_LEN + 1) > MAX_ADDRESS_LEN) { > + fprintf(stderr, "ip address too long\n"); > + if (closingBracket) > + *closingBracket = ']'; > + return EX_USAGE; > + } > + > + strcpy(parsed_info->addrlist, value); > + if (parsed_info->verboseflag) > + fprintf(stderr, "ip address %s override specified\n", value); > + > + /* put back closing bracket */ > + if (closingBracket) > + *closingBracket = ']'; > + > + return 0; > +} > + > static int > parse_options(const char *data, struct parsed_mount_info *parsed_info) > { > @@ -861,23 +902,10 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info) > break; > > case OPT_IP: > - if (!value || !*value) { > - fprintf(stderr, > - "target ip address argument missing\n"); > - } else if (strnlen(value, MAX_ADDRESS_LEN) <= > - MAX_ADDRESS_LEN) { > - strcpy(parsed_info->addrlist, value); > - if (parsed_info->verboseflag) > - fprintf(stderr, > - "ip address %s override specified\n", > - value); > - goto nocopy; > - } else { > - fprintf(stderr, "ip address too long\n"); > - return EX_USAGE; > - > - } > - break; > + rc = parse_ip(value, parsed_info); > + if (rc) > + return rc; > + goto nocopy; > > /* unc || target || path */ > case OPT_UNC: > -- > 1.7.10.4 > Ping. Any comments? -- Peace and Blessings, -Scott. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 22 Apr 2013 10:21:13 -0400 Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > On Mon, Apr 15, 2013 at 9:29 PM, <scott.lovenberg@gmail.com> wrote: > > > > From: Scott Lovenberg <scott.lovenberg@gmail.com> > > > > This is a respin of the last patch that returns EX_USAGE rather than OPT_ERROR as per Jeff's suggestion. > > > > Signed-off-by: Scott Lovenberg <scott.lovenberg@gmail.com> > > --- > > AUTHORS | 1 + > > mount.cifs.c | 62 ++++++++++++++++++++++++++++++++++++++++++---------------- > > 2 files changed, 46 insertions(+), 17 deletions(-) > > > > diff --git a/AUTHORS b/AUTHORS > > index 2807079..081c2fe 100644 > > --- a/AUTHORS > > +++ b/AUTHORS > > @@ -5,5 +5,6 @@ Shirish Pargaonkar <shirishpargaonkar@gmail.com> > > Suresh Jayaraman <sjayaraman@suse.de> > > Pavel Shilovsky <piastry@etersoft.ru> > > Igor Druzhinin <jaxbrigs@gmail.com> > > +Scott Lovenberg <scott.lovenberg@gmail.com> > > > > ...and others. > > diff --git a/mount.cifs.c b/mount.cifs.c > > index 3b2b89e..561517b 100644 > > --- a/mount.cifs.c > > +++ b/mount.cifs.c > > @@ -751,6 +751,47 @@ static int parse_opt_token(const char *token) > > return OPT_ERROR; > > } > > > > +/* > > + Extract an ipv4 or ipv6 address and put it into parsed_info. > > + Returns 0 on success. > > +*/ > > +static int parse_ip(char *value, struct parsed_mount_info *parsed_info) > > +{ > > + char *closingBracket = NULL; > > + > > + if (!value || !*value) { > > + fprintf(stderr, "target ip address argument missing\n"); > > + return EX_USAGE; > > + } > > + > > + /* check for ipv6 in brackets */ > > + if (value[0] == '[') { > > + value++; > > + /* temporarily null the closing bracket */ > > + closingBracket = strchr(value, ']'); > > + if (closingBracket) > > + *closingBracket = '\0'; > > + } > > + > > + /* check length of address */ > > + if (strnlen(value, MAX_ADDRESS_LEN + 1) > MAX_ADDRESS_LEN) { > > + fprintf(stderr, "ip address too long\n"); > > + if (closingBracket) > > + *closingBracket = ']'; > > + return EX_USAGE; > > + } > > + > > + strcpy(parsed_info->addrlist, value); > > + if (parsed_info->verboseflag) > > + fprintf(stderr, "ip address %s override specified\n", value); > > + > > + /* put back closing bracket */ > > + if (closingBracket) > > + *closingBracket = ']'; > > + > > + return 0; > > +} > > + > > static int > > parse_options(const char *data, struct parsed_mount_info *parsed_info) > > { > > @@ -861,23 +902,10 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info) > > break; > > > > case OPT_IP: > > - if (!value || !*value) { > > - fprintf(stderr, > > - "target ip address argument missing\n"); > > - } else if (strnlen(value, MAX_ADDRESS_LEN) <= > > - MAX_ADDRESS_LEN) { > > - strcpy(parsed_info->addrlist, value); > > - if (parsed_info->verboseflag) > > - fprintf(stderr, > > - "ip address %s override specified\n", > > - value); > > - goto nocopy; > > - } else { > > - fprintf(stderr, "ip address too long\n"); > > - return EX_USAGE; > > - > > - } > > - break; > > + rc = parse_ip(value, parsed_info); > > + if (rc) > > + return rc; > > + goto nocopy; > > > > /* unc || target || path */ > > case OPT_UNC: > > -- > > 1.7.10.4 > > > > > Ping. > Any comments? > I guess I don't quite understand why we need this patch. For NFS it's pretty clear that we would since ':' has always served as the delimiter between the hostname and the pathname. For CIFS, not so much... IPv6 addresses typically don't have '/' or '\\' characters in them, so why do we need to wrap them in []? Note that this syntax doesn't follow what Windows does either, AFAIK. There, you need something like this since UNC paths can't contain ':' characters (and maybe also '%'): http://www.samba.org/~idra/code/nss-ipv6literal/README.html ...we have no such restriction under Linux though.
On Mon, Apr 22, 2013 at 11:09 AM, Jeff Layton <jlayton@samba.org> wrote: >> Ping. >> Any comments? >> > > I guess I don't quite understand why we need this patch. For NFS it's > pretty clear that we would since ':' has always served as the delimiter > between the hostname and the pathname. > > For CIFS, not so much... IPv6 addresses typically don't have '/' or '\\' > characters in them, so why do we need to wrap them in []? > > Note that this syntax doesn't follow what Windows does either, AFAIK. > There, you need something like this since UNC paths can't contain ':' > characters (and maybe also '%'): > > http://www.samba.org/~idra/code/nss-ipv6literal/README.html > > ...we have no such restriction under Linux though. > I liked this approach for two reasons. It handles a convention for IPv6 formatting that is used that we don't support and allows us to (in the future) parse an address from the path instead of having to use --ip=, which I feel is a bit cumbersome. Originally I was going to just put in support for something like "mount.cifs //IPv6Address/share /mnt/path" right away, but I wanted to get this part in first. -- Peace and Blessings, -Scott. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 23 Apr 2013 08:34:14 -0400 Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > On Mon, Apr 22, 2013 at 11:09 AM, Jeff Layton <jlayton@samba.org> wrote: > >> Ping. > >> Any comments? > >> > > > > I guess I don't quite understand why we need this patch. For NFS it's > > pretty clear that we would since ':' has always served as the delimiter > > between the hostname and the pathname. > > > > For CIFS, not so much... IPv6 addresses typically don't have '/' or '\\' > > characters in them, so why do we need to wrap them in []? > > > > Note that this syntax doesn't follow what Windows does either, AFAIK. > > There, you need something like this since UNC paths can't contain ':' > > characters (and maybe also '%'): > > > > http://www.samba.org/~idra/code/nss-ipv6literal/README.html > > > > ...we have no such restriction under Linux though. > > > > I liked this approach for two reasons. It handles a convention for > IPv6 formatting that is used that we don't support and allows us to > (in the future) parse an address from the path instead of having to > use --ip=, which I feel is a bit cumbersome. Originally I was going > to just put in support for something like "mount.cifs > //IPv6Address/share /mnt/path" right away, but I wanted to get this > part in first. > Again, I'm confused...this sort of syntax seems to work just fine already: # mount -t cifs //fecd::1/share /mnt/cifs -o sec=none,multiuser Also, --ip= is not a valid option in the longopts[] array in mount.cifs (maybe you mean -o ip=). I still don't see what benefit we receive from allowing someone specify addresses inside of square brackets.
On Tue, Apr 23, 2013 at 10:01 AM, Jeff Layton <jlayton@samba.org> wrote: > On Tue, 23 Apr 2013 08:34:14 -0400 > Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > >> On Mon, Apr 22, 2013 at 11:09 AM, Jeff Layton <jlayton@samba.org> wrote: >> >> Ping. >> >> Any comments? >> >> >> > >> > I guess I don't quite understand why we need this patch. For NFS it's >> > pretty clear that we would since ':' has always served as the delimiter >> > between the hostname and the pathname. >> > >> > For CIFS, not so much... IPv6 addresses typically don't have '/' or '\\' >> > characters in them, so why do we need to wrap them in []? >> > >> > Note that this syntax doesn't follow what Windows does either, AFAIK. >> > There, you need something like this since UNC paths can't contain ':' >> > characters (and maybe also '%'): >> > >> > http://www.samba.org/~idra/code/nss-ipv6literal/README.html >> > >> > ...we have no such restriction under Linux though. >> > >> >> I liked this approach for two reasons. It handles a convention for >> IPv6 formatting that is used that we don't support and allows us to >> (in the future) parse an address from the path instead of having to >> use --ip=, which I feel is a bit cumbersome. Originally I was going >> to just put in support for something like "mount.cifs >> //IPv6Address/share /mnt/path" right away, but I wanted to get this >> part in first. >> > > Again, I'm confused...this sort of syntax seems to work just fine > already: > > # mount -t cifs //fecd::1/share /mnt/cifs -o sec=none,multiuser Hrm, I thought that didn't work (or, more likely, my test environment had some other problem). > > Also, --ip= is not a valid option in the longopts[] array in > mount.cifs (maybe you mean -o ip=). I still don't see what benefit we > receive from allowing someone specify addresses inside of square > brackets. Sorry, posted before my first cup of coffee; -o ip= is what I meant to say. Since this already works, I'm inclined to agree that this patch brings nothing to the table. -- Peace and Blessings, -Scott. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 23, 2013 at 9:01 AM, Jeff Layton <jlayton@samba.org> wrote: > On Tue, 23 Apr 2013 08:34:14 -0400 > Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > >> On Mon, Apr 22, 2013 at 11:09 AM, Jeff Layton <jlayton@samba.org> wrote: >> >> Ping. >> >> Any comments? >> >> >> > >> > I guess I don't quite understand why we need this patch. For NFS it's >> > pretty clear that we would since ':' has always served as the delimiter >> > between the hostname and the pathname. I am ok with the idea of adding additional support that looks more like what an NFS user is comfortable with. Ideally a user should be able to type very similar mount commands for cifs/smb3 and NFSv3/v4 etc. Scott, Can you give a list of an example of these - mounts with each of the IPv6 formats that work (and what doesn't work) along with the one you propose? >> > For CIFS, not so much... IPv6 addresses typically don't have '/' or '\\' >> > characters in them, so why do we need to wrap them in []? >> > >> > Note that this syntax doesn't follow what Windows does either, AFAIK. >> > There, you need something like this since UNC paths can't contain ':' >> > characters (and maybe also '%'): >> > >> > http://www.samba.org/~idra/code/nss-ipv6literal/README.html >> > >> > ...we have no such restriction under Linux though. >> > >> >> I liked this approach for two reasons. It handles a convention for >> IPv6 formatting that is used that we don't support and allows us to >> (in the future) parse an address from the path instead of having to >> use --ip=, which I feel is a bit cumbersome. Originally I was going >> to just put in support for something like "mount.cifs >> //IPv6Address/share /mnt/path" right away, but I wanted to get this >> part in first. >> > > Again, I'm confused...this sort of syntax seems to work just fine > already: > > # mount -t cifs //fecd::1/share /mnt/cifs -o sec=none,multiuser > > Also, --ip= is not a valid option in the longopts[] array in > mount.cifs (maybe you mean -o ip=). I still don't see what benefit we > receive from allowing someone specify addresses inside of square > brackets. > > -- > Jeff Layton <jlayton@samba.org>
On Tue, Apr 23, 2013 at 10:26 AM, Steve French <smfrench@gmail.com> wrote: > On Tue, Apr 23, 2013 at 9:01 AM, Jeff Layton <jlayton@samba.org> wrote: >>> > I guess I don't quite understand why we need this patch. For NFS it's >>> > pretty clear that we would since ':' has always served as the delimiter >>> > between the hostname and the pathname. > > I am ok with the idea of adding additional support that looks more > like what an NFS user is comfortable with. Ideally a user should be > able to type very similar mount commands for cifs/smb3 and NFSv3/v4 > etc. > > Scott, > Can you give a list of an example of these - mounts with each of the > IPv6 formats that work (and what doesn't work) along with the one you > propose? I can get that together. -- Peace and Blessings, -Scott. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/AUTHORS b/AUTHORS index 2807079..081c2fe 100644 --- a/AUTHORS +++ b/AUTHORS @@ -5,5 +5,6 @@ Shirish Pargaonkar <shirishpargaonkar@gmail.com> Suresh Jayaraman <sjayaraman@suse.de> Pavel Shilovsky <piastry@etersoft.ru> Igor Druzhinin <jaxbrigs@gmail.com> +Scott Lovenberg <scott.lovenberg@gmail.com> ...and others. diff --git a/mount.cifs.c b/mount.cifs.c index 3b2b89e..561517b 100644 --- a/mount.cifs.c +++ b/mount.cifs.c @@ -751,6 +751,47 @@ static int parse_opt_token(const char *token) return OPT_ERROR; } +/* + Extract an ipv4 or ipv6 address and put it into parsed_info. + Returns 0 on success. +*/ +static int parse_ip(char *value, struct parsed_mount_info *parsed_info) +{ + char *closingBracket = NULL; + + if (!value || !*value) { + fprintf(stderr, "target ip address argument missing\n"); + return EX_USAGE; + } + + /* check for ipv6 in brackets */ + if (value[0] == '[') { + value++; + /* temporarily null the closing bracket */ + closingBracket = strchr(value, ']'); + if (closingBracket) + *closingBracket = '\0'; + } + + /* check length of address */ + if (strnlen(value, MAX_ADDRESS_LEN + 1) > MAX_ADDRESS_LEN) { + fprintf(stderr, "ip address too long\n"); + if (closingBracket) + *closingBracket = ']'; + return EX_USAGE; + } + + strcpy(parsed_info->addrlist, value); + if (parsed_info->verboseflag) + fprintf(stderr, "ip address %s override specified\n", value); + + /* put back closing bracket */ + if (closingBracket) + *closingBracket = ']'; + + return 0; +} + static int parse_options(const char *data, struct parsed_mount_info *parsed_info) { @@ -861,23 +902,10 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info) break; case OPT_IP: - if (!value || !*value) { - fprintf(stderr, - "target ip address argument missing\n"); - } else if (strnlen(value, MAX_ADDRESS_LEN) <= - MAX_ADDRESS_LEN) { - strcpy(parsed_info->addrlist, value); - if (parsed_info->verboseflag) - fprintf(stderr, - "ip address %s override specified\n", - value); - goto nocopy; - } else { - fprintf(stderr, "ip address too long\n"); - return EX_USAGE; - - } - break; + rc = parse_ip(value, parsed_info); + if (rc) + return rc; + goto nocopy; /* unc || target || path */ case OPT_UNC: