[Samba] [PATCH] Add warning that NFS syntax is deprecated and will be removed in cifs-utils-6.0.

[scott.l...@gmail.com]

From: Scott Lovenberg <scott.l...@gmail.com>

Signed-off-by: Scott Lovenberg <scott.l...@gmail.com>
---
mount.cifs.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 756fce2..061ce32 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -1335,6 +1335,7 @@ static int parse_unc(const char *unc_name, struct parsed_mount_info *parsed_info
}

/* Set up "host" and "share" pointers based on UNC format. */
+ /* TODO: Remove support for NFS syntax as of cifs-utils-6.0. */
if (strncmp(unc_name, "//", 2) && strncmp(unc_name, "\\\\", 2)) {
/*
* check for nfs syntax (server:/share/prepath)
@@ -1351,6 +1352,9 @@ static int parse_unc(const char *unc_name, struct parsed_mount_info *parsed_info
share++;
if (*share == '/')
++share;
+ fprintf(stderr, "WARNING: using NFS syntax for mounting CIFS "
+ "shares is deprecated and will be removed in cifs-utils"
+ "-6.0. Please migrate to UNC syntax.");
} else {
host = unc_name + 2;
hostlen = strcspn(host, "/\\");
--
1.7.5.4

--
To unsubscribe from this list go to the following URL and read the
instructions: https://lists.samba.org/mailman/options/samba

[scott.l...@gmail.com]

The following patch adds a warning when using NFS mounting syntax (server:/share), instead of the usual UNC syntax (//server/share || \\server\share), that support for NFS style mounts will be removed in version 6.0 of the mount.cifs utility.

The reasoning for this is simple. Support for NFS syntax is undocumented and increases maintenance overhead. This came up recently on the cifs-utils list when discussing how to handle mounting a share NFS style using an IPv6 address. Since the ':' character is valid in a POSIX file path or share name it is an ambiguous delimiter. Consider the following valid server share : "dead:beef::1:iSCSIExportedByIQN:storage".

Instead of adding complicated code to the parser to support an undocumented feature, we're optin
g to remove the feature in the mount utility in version 6.0 if there is no objection.

[Scott Lovenberg]

On 10/18/2012 1:50 PM, scott.l...@gmail.com wrote:
> From: Scott Lovenberg<scott.l...@gmail.com>
>
> Signed-off-by: Scott Lovenberg<scott.l...@gmail.com>
> ---
> mount.cifs.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 756fce2..061ce32 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -1335,6 +1335,7 @@ static int parse_unc(const char *unc_name, struct parsed_mount_info *parsed_info
> }
>
> /* Set up "host" and "share" pointers based on UNC format. */
> + /* TODO: Remove support for NFS syntax as of cifs-utils-6.0. */

> if (strncmp(unc_name, "//", 2)&& strncmp(unc_name, "\\\\", 2)) {

> /*
> * check for nfs syntax (server:/share/prepath)
> @@ -1351,6 +1352,9 @@ static int parse_unc(const char *unc_name, struct parsed_mount_info *parsed_info
> share++;
> if (*share == '/')
> ++share;
> + fprintf(stderr, "WARNING: using NFS syntax for mounting CIFS "
> + "shares is deprecated and will be removed in cifs-utils"
> + "-6.0. Please migrate to UNC syntax.");
> } else {
> host = unc_name + 2;
> hostlen = strcspn(host, "/\\");

Sorry, git send-email just blew up in my face. It was supposed to send
a first email that explained the patch. Of course it worked perfectly
when I tested it to my own email address. I'll figure out why the first
message is missing and repost. Sorry for the noise.

[scott.l...@gmail.com]

From: Scott Lovenberg <scott.l...@gmail.com>

Signed-off-by: Scott Lovenberg <scott.l...@gmail.com>
---
mount.cifs.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 756fce2..061ce32 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -1335,6 +1335,7 @@ static int parse_unc(const char *unc_name, struct parsed_mount_info *parsed_info
}

/* Set up "host" and "share" pointers based on UNC format. */
+ /* TODO: Remove support for NFS syntax as of cifs-utils-6.0. */

if (strncmp(unc_name, "//", 2) && strncmp(unc_name, "\\\\", 2)) {

/*
* check for nfs syntax (server:/share/prepath)
@@ -1351,6 +1352,9 @@ static int parse_unc(const char *unc_name, struct parsed_mount_info *parsed_info
share++;
if (*share == '/')
++share;
+ fprintf(stderr, "WARNING: using NFS syntax for mounting CIFS "
+ "shares is deprecated and will be removed in cifs-utils"
+ "-6.0. Please migrate to UNC syntax.");
} else {
host = unc_name + 2;
hostlen = strcspn(host, "/\\");

--
1.7.5.4

[scott.l...@gmail.com]

This patch adds a warning when using NFS mounting syntax (server:/share), instead of the usual UNC syntax (//server/share || \\server\share), that support for NFS style mounts will be removed in version 6.0 of the mount.cifs utility.

The reasoning for this is simple. Support for NFS syntax is undocumented and increases maintenance overhead. This came up recently on the cifs-utils list when discussing how to handle mounting a share NFS style using an IPv6 address. Since the ':' character is valid in a POSIX file path or share name it is an ambiguous delimiter. Consider the following valid server share : "dead:beef::1:iSCSIExportedByIQN:storage".

Instead of adding complicated code to the parser to support an undocumented feature, we're optin
g to remove the feature in the mount utility in version 6.0 if there is no objection.

[Scott Lovenberg]

On 10/18/2012 2:07 PM, scott.l...@gmail.com wrote:
> This patch adds a warning when using NFS mounting syntax (server:/share), instead of the usual UNC syntax (//server/share || \\server\share), that support for NFS style mounts will be removed in version 6.0 of the mount.cifs utility.
>
> The reasoning for this is simple. Support for NFS syntax is undocumented and increases maintenance overhead. This came up recently on the cifs-utils list when discussing how to handle mounting a share NFS style using an IPv6 address. Since the ':' character is valid in a POSIX file path or share name it is an ambiguous delimiter. Consider the following valid server share : "dead:beef::1:iSCSIExportedByIQN:storage".
>
> Instead of adding complicated code to the parser to support an undocumented feature, we're optin
> g to remove the feature in the mount utility in version 6.0 if there is no objection.
>
>

Jeff, it's been a few days and no one has objected (or really said
anything). Can we merge this patch?

[steve]

On 10/23/2012 05:56 PM, Scott Lovenberg wrote:
> On 10/18/2012 2:07 PM, scott.l...@gmail.com wrote:
> no one has objected (or really said anything). Can we merge this patch?
> --

Hi
I'm just trying to represent users. Can we take this to user level by
giving an example of what will work and what will not work after the patch?

For example, the Linux automounter.

Currently, we have this map:
* -fstype=cifs,rw,sec=krb5 ://myserver/myshare/&

Are you talking about the difference between that and this:
* -fstype=cifs,rw,sec=krb5 myserver:/myshare/&

Question: will I need to change anything due to this patch?

Cheers,
Steve

[Jeff Layton]

On Tue, 23 Oct 2012 18:47:37 +0200
steve <st...@steve-ss.com> wrote:

> On 10/23/2012 05:56 PM, Scott Lovenberg wrote:
> > On 10/18/2012 2:07 PM, scott.l...@gmail.com wrote:
> > no one has objected (or really said anything). Can we merge this patch?
> > --
> Hi
> I'm just trying to represent users. Can we take this to user level by
> giving an example of what will work and what will not work after the patch?
>
> For example, the Linux automounter.
>
> Currently, we have this map:
> * -fstype=cifs,rw,sec=krb5 ://myserver/myshare/&
>

Does that really work? What purpose does the ':' serve there? That
should probably be removed. I doubt we'd end up breaking that syntax,
but I can't be certain.

> Are you talking about the difference between that and this:
> * -fstype=cifs,rw,sec=krb5 myserver:/myshare/&

Right, the above syntax would no longer work after the change.

>
> Question: will I need to change anything due to this patch?
>

For this patch, you don't need to do anything. It just adds a warning.
Eventually though, nfs-style "devicenames" would no longer work for
cifs mounts. For your map above, you probably want something like:

* -fstype=cifs,rw,sec=krb5 //myserver/myshare/&

(i.e. get rid of the extraneous ':').
--
Jeff Layton <jla...@redhat.com>

[steve]

On 10/23/2012 07:02 PM, Jeff Layton wrote:
> On Tue, 23 Oct 2012 18:47:37 +0200
> steve <st...@steve-ss.com> wrote:
>
>> On 10/23/2012 05:56 PM, Scott Lovenberg wrote:
>> Currently, we have this map: * -fstype=cifs,rw,sec=krb5
>> ://myserver/myshare/&
> Does that really work? What purpose does the ':' serve there?

Yes. They always put a ':' before the mount except for the default NFS.
I took a look at the example /etc/auto.misc which comes (commented out)
with openSUSE. They always put a ':'.

> That
> should probably be removed. I doubt we'd end up breaking that syntax,
> but I can't be certain.
>

Just to say that this is a seemingly innocuous patch, but one which may
lead to confusion.
HTH,
Steve

[Scott Lovenberg]

On Tue, Oct 23, 2012 at 12:47 PM, steve <st...@steve-ss.com> wrote:
> On 10/23/2012 05:56 PM, Scott Lovenberg wrote:
>>
>> On 10/18/2012 2:07 PM, scott.l...@gmail.com wrote:
>> no one has objected (or really said anything). Can we merge this patch?
>> --
>
> Hi
> I'm just trying to represent users. Can we take this to user level by giving
> an example of what will work and what will not work after the patch?
>

I should clarify, this patch doesn't change the behavior of the mount
utility, it just warns the user that in future releases the syntax
that they are using will be removed. The patch to remove the behavior
is going to be in a later release.

What will work is any path that begins with "//" or "\\" which is a
normal UNC. So your normal "//server/share" path is fine. NFS syntax
allows for you to specify the path like "server:/share". That syntax
will no longer work in cifs-utils 6.0.

> For example, the Linux automounter.
>
> Currently, we have this map:
> * -fstype=cifs,rw,sec=krb5 ://myserver/myshare/&
>
> Are you talking about the difference between that and this:
> * -fstype=cifs,rw,sec=krb5 myserver:/myshare/&
>
> Question: will I need to change anything due to this patch?
>

Quite the opposite, the "//myserver/myshare" is correct,
"myserver:/myshare" will no longer work. The ':' is part of the
automounter's map syntax. It will use the path "//myserver/myshare".

--
Peace and Blessings,
-Scott.

[Jeff Layton]

On Tue, 23 Oct 2012 19:22:32 +0200

steve <st...@steve-ss.com> wrote:

> On 10/23/2012 07:02 PM, Jeff Layton wrote:
> > On Tue, 23 Oct 2012 18:47:37 +0200
> > steve <st...@steve-ss.com> wrote:
> >
> >> On 10/23/2012 05:56 PM, Scott Lovenberg wrote:
> >> Currently, we have this map: * -fstype=cifs,rw,sec=krb5
> >> ://myserver/myshare/&
> > Does that really work? What purpose does the ':' serve there?
> Yes. They always put a ':' before the mount except for the default NFS.
> I took a look at the example /etc/auto.misc which comes (commented out)
> with openSUSE. They always put a ':'.

Ok, I see now. From autofs(5):

If the filesystem to be mounted begins with a / (such as local /dev
entries or smbfs shares) a : needs to be prefixed (e.g. :/dev/sda1).

...I guess it's necessary for the autofs parser. I assume that the ':'
doesn't get passed to the actual mount invocation though, so that
should continue to work just fine.

> > That
> > should probably be removed. I doubt we'd end up breaking that syntax,
> > but I can't be certain.
> >
> Just to say that this is a seemingly innocuous patch, but one which may
> lead to confusion.

Well, better confusion now than confusion when it breaks. cifs really
is just too "loose" about the syntax of things that it accepts, which
sounds great until you have to test all of the different variations...

--
Jeff Layton <jla...@redhat.com>

[Scott Lovenberg]

On Tue, Oct 23, 2012 at 1:22 PM, steve <st...@steve-ss.com> wrote:
> On 10/23/2012 07:02 PM, Jeff Layton wrote:
>>
>> On Tue, 23 Oct 2012 18:47:37 +0200
>> steve <st...@steve-ss.com> wrote:
>>
>>> On 10/23/2012 05:56 PM, Scott Lovenberg wrote:
>>> Currently, we have this map: * -fstype=cifs,rw,sec=krb5
>>> ://myserver/myshare/&
>>
>> Does that really work? What purpose does the ':' serve there?
>
> Yes. They always put a ':' before the mount except for the default NFS. I
> took a look at the example /etc/auto.misc which comes (commented out) with
> openSUSE. They always put a ':'.

I double checked this. The ':' is a token for the automounter that
tells it that it's a local device. You could probably remove that
character. http://www.faqs.org/docs/Linux-mini/Automount.html#s4

--
Peace and Blessings,
-Scott.

[steve]

Hi Scott, hi everyone
Yeah, that's fine.
Does this clear up the issue with the ':'? I should have made it clearer
that I was referring to autofs and not mounting e.g. from fstab. I just
tried the automounter on cifs without the ':' and it doesn't work.

Would it perhaps help to put a message in the logs when it fails, rather
than silence? Or maybe that's more of a question for the autofs guys.
Cheers,
Steve

[Jeff Layton]

Merged (with addition of a newline to the end of warning message)...
--
Jeff Layton <jla...@redhat.com>

[Scott Lovenberg]

On Tue, Oct 23, 2012 at 3:23 PM, steve <st...@steve-ss.com> wrote:

> Hi Scott, hi everyone
> Yeah, that's fine.
> Does this clear up the issue with the ':'? I should have made it clearer
> that I was referring to autofs and not mounting e.g. from fstab. I just
> tried the automounter on cifs without the ':' and it doesn't work.
>
> Would it perhaps help to put a message in the logs when it fails, rather
> than silence? Or maybe that's more of a question for the autofs guys.
> Cheers,
> Steve
>

I've been at home thinking about this for a while tonight. I've
checked the documentation for autofs and they do what they say what
they'll do with that path (treat anything without a ':' as an NFS
mount). On our side, (mount.cifs) we do what we say we'll do (support
UNC paths). The most we could ask of them is to add/modify their
documentation to include the case for CIFS instead of just SMB.

This doesn't change anything on the mount.cifs side other than
explicitly directing users to the correct syntax for CIFS shares when
using autofs. Ultimately the autofs documentation implicitly states
that CIFS shares should use a ':'.

All that being said, the mount.cifs has never officially supported NFS
path syntax. We aren't silently ignoring the issue; we're sending a
warning to stdout that in a future version of the mount utility we
won't support this undocumented behavior. To be fair, that's more
than most code bases do for deprecating undocumented features.

If anyone wants me to pursue the issue, I'll see what I can do about
getting the documentation for autofs altered to explicitly mention
CIFS paths. I think that is reasonable for everyone. It's after 2 AM
in my part of the world, so I'll do this tomorrow after my first cup
of coffee if anyone requests it.

--
Peace and Blessings,
-Scott.

[steve]

On 23/10/12 19:36, Jeff Layton wrote:
> On Tue, 23 Oct 2012 19:22:32 +0200
> steve <st...@steve-ss.com> wrote:
>
>> On 10/23/2012 07:02 PM, Jeff Layton wrote:
>>> On Tue, 23 Oct 2012 18:47:37 +0200
>>> steve <st...@steve-ss.com> wrote:
>>>
>>>> On 10/23/2012 05:56 PM, Scott Lovenberg wrote:

>
> Well, better confusion now than confusion when it breaks. cifs really
> is just too "loose" about the syntax of things that it accepts, which
> sounds great until you have to test all of the different variations...
>

Hi
As dev's you have the power to change that. Users need to have it 'just
work'. Simply tell us what the syntax is and we'll stick to it gladly.
All we need is documentation which says, in plain straightforward
English, something like:

This is the syntax allowed:
(say it in dev terms with all [:\\|//{ sort of stuff if you like)

And here are some real examples:
mount -t cifs //server/share /mnt
etc, etc.

Just make it clear and please don't give alternatives.
Cheers,
Steve