Re: [PATCH 02/16] CIFS: Introduce SMB2 mounts as vers=2

[Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Sun, 6 May 2012 08:31:03 -0400
Jeff Layton <jlayton@samba.org> wrote:

> On Mon, 26 Mar 2012 13:21:29 +0400
> Pavel Shilovsky <piastry@etersoft.ru> wrote:
> 
> > From: Steve French <sfrench@us.ibm.com>
> > 
> > As with Linux nfs client, which uses "nfsvers=" or "vers=" to
> > indicate which protocol to use for mount, specifying
> > 
> > "vers=smb2" or "vers=2"
> > 
> > will force an SMB2 mount. When vers is not specified CIFS is used
> > 
> > ie "vers=cifs" or "vers=1"
> > 
> > We can eventually autonegotiate down from SMB2 to CIFS
> > when SMB2 is stable enough to make it the default, but this
> > is for the future.  At that time we could also implement a
> > "maxprotocol" mount option as smbclient and Samba have today,
> > but that would be premature until SMB2 is stable.
> > 
> > Intially the SMB2 Kconfig option will depend on "BROKEN"
> > until the merge is complete, and then be "EXPERIMENTAL"
> > When it is no longer experimental we can consider changing
> > the default protocol to attempt first.
> > 
> > Reviewed-by: Pavel Shilovsky <piastry@etersoft.ru>
> > Signed-off-by: Steve French <sfrench@us.ibm.com>
> > ---
> >  fs/cifs/cifsglob.h |    3 +++
> >  fs/cifs/connect.c  |    8 ++++++++
> >  2 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 4ff6313..9e28070 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -167,6 +167,9 @@ struct smb_vol {
> >  	umode_t file_mode;
> >  	umode_t dir_mode;
> >  	unsigned secFlg;
> > +#ifdef CONFIG_CIFS_SMB2
> > +	bool use_smb2:1; /* use SMB2 protocol rather than CIFS */
> > +#endif
> 
> Now that we have a new SMB major version to deal with, a boolean really
> seems unsuited to this purpose. Should we change this to be an unsigned
> int or something and assign the correct version number to it?
> 
> >  	bool retry:1;
> >  	bool intr:1;
> >  	bool setuids:1;
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 9808154..955f0a3 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -1856,6 +1856,14 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
> >  			    strnicmp(string, "1", 1) == 0) {
> >  				/* This is the default */
> >  				break;
> > +			} else if (strnicmp(string, "smb2", 4) == 0 ||
> > +				   strnicmp(string, "2", 1) == 0) {
> > +#ifdef CONFIG_CIFS_SMB2
> > +				vol->use_smb2 = true;
> > +#else
> > +				cERROR(1, "SMB2 support not enabled");
> 
> I think we should not try to accept ver=smb2 or anything. Let's stick
> with numbered versions. In fact, the ver=cifs that the current code
> allows should also be removed, IMO. Nothing sends that now, and we
> shouldn't establish the precedent.
> 
> > +#endif /* CONFIG_CIFS_SMB2 */
> > +				break;
> >  			}
> >  			/* For all other value, error */
> >  			printk(KERN_WARNING "CIFS: Invalid version"
> 

Carrying this thought a bit further, I think we need to step back and
consider what this should look like from a user's standpoint:

After all, what does "ver=2" mean over the long-term? 2.002? 2.100?

Perhaps we should not accept "ver=2" and require an explicit "ver=2.0"
or "ver=2.1"? Ditto with 3.0?

- -- 
Jeff Layton <jlayton@samba.org>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.18 (GNU/Linux)

iQIcBAEBAgAGBQJPpnChAAoJEAAOaEEZVoIVyQ0QAKHFOXP3DnMTXocBYBPm+5Dk
EcY8zXexnWLsBIPzjADX03haZF9nQFDxlmEzmFSC6kIXlx7eXIwE/WTFL7oij5ff
TqMSOqZEqxSPZzyTf3JxFqK6mL75xrC4YvLLU2UHUc1HkCWHEqyRflCu+Ksa5Zoq
b95d0mZAsYL4rOT18zOPVQuirRe41U/MNSIy+gMQOZqSegFshGY0XHj7w+qlAmJN
WkxakJm4cth1KTirKJnSiBlV57ziHLFSTOlV1eLIOyoTZJ5Mgg9QENPhris0Ct45
2fqAiu09JvFZsHs0eJpf0qlvZwmU8RMiPTYVRLl6HjD5pZOFcmXUUrqcRvjTmg1h
aUbjfxfryKPaKbiQnQNtt1f0LPjwjJrA7zuh4kHLlS3DOl341V0YzMWGQ4FGC4oR
aub3LBKAa/pJcgOu6DP7R5r87q3J3f2XS8ARQpQcpgHcz5y/ugVQi5i2FVC31Es1
J57+aZM4cVyqyFIs2MB1/aOTJOlNNxLYt26BQ3KPg/+j8ffK7pntiuTKLk5isXYA
F4O9djeRXoXQY9cOJ6naG3l0LJ7y51JGD4pBpYBTDS7SaGCNdiImuoxIgK1eldlo
zWmCfullq3S0bad8AWa+YnoAnV1v3y9poMRHVvYgV3i/qRPvQW+icy1dEAsPfa7W
CN7ZIupx9/Hjc68scd+e
=Yz2Z
-----END PGP SIGNATURE-----