On Fri, 22 Nov 2013 11:26:58 +0100
Gregor Beck <gbeck@xxxxxxxxx> wrote:
> ---
> cifsglob.h | 1 +
> smb1ops.c | 26 ++++++++++++++++++--------
> 2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/cifsglob.h b/cifsglob.h
> index 52ca861..d3adde7 100644
> --- a/cifsglob.h
> +++ b/cifsglob.h
> @@ -818,6 +818,7 @@ struct cifs_tcon {
> bool local_lease:1; /* check leases (only) on local system not remote */
> bool broken_posix_open; /* e.g. Samba server versions < 3.3.2, 3.2.9 */
> bool need_reconnect:1; /* connection reset, tid now invalid */
> + bool broken_qpath_info:1;
> #ifdef CONFIG_CIFS_SMB2
> bool print:1; /* set if connection to printer share */
> bool bad_network_name:1; /* set if ret status STATUS_BAD_NETWORK_NAME */
> diff --git a/smb1ops.c b/smb1ops.c
> index 11daa28..e3798a4 100644
> --- a/smb1ops.c
> +++ b/smb1ops.c
> @@ -511,7 +511,7 @@ static int
> cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb, const char *full_path)
> {
> - int rc;
> + int rc= -EIO;
> FILE_ALL_INFO *file_info;
> const int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;
>
> @@ -519,13 +519,18 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> if (file_info == NULL)
> return -ENOMEM;
>
> - rc = CIFSSMBQPathInfo(xid, tcon, full_path, file_info,
> - 0 /* not legacy */, cifs_sb->local_nls, remap);
> + if (!tcon->broken_qpath_info) {
> + rc = CIFSSMBQPathInfo(xid, tcon, full_path, file_info,
> + 0, cifs_sb->local_nls, remap);
> + }
> if (rc == -EIO) {
> - cifs_dbg(FYI, "is_path_accessible: FALLBACK");
> rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
> (FILE_BASIC_INFO*)file_info,
> cifs_sb->local_nls, remap);
> + cifs_dbg(FYI, "is_path_accessible: FALLBACK returns %d", rc);
> + if (!rc) {
> + tcon->broken_qpath_info = true;
> + }
> }
>
> if (rc == -EOPNOTSUPP || rc == -EINVAL)
> @@ -540,15 +545,16 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb, const char *full_path,
> FILE_ALL_INFO *data, bool *adjustTZ)
> {
> - int rc;
> + int rc = -EIO;
> const int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;
>
> /* could do find first instead but this returns more info */
> - rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
> - cifs_sb->local_nls, remap);
> + if (!tcon->broken_qpath_info) {
> + rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0,
> + cifs_sb->local_nls, remap);
> + }
>
> if (rc == -EIO) {
> - cifs_dbg(FYI, "cifs_query_path_info: FALLBACK");
> rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
> (FILE_BASIC_INFO*)data,
> cifs_sb->local_nls, remap);
> @@ -557,6 +563,10 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> (void*)data + sizeof(FILE_BASIC_INFO),
> cifs_sb->local_nls, remap);
> }
> + cifs_dbg(FYI, "cifs_query_path_info: FALLBACK returns %d", rc);
> + if (!rc) {
> + tcon->broken_qpath_info = true;
> + }
I have a bit of a problem with the above...
Trying to change behavior automatically like this is fraught with peril
and cifs.ko is already a horrid mess of this sort of thing. It's bad
enough that we have some existing fallbacks for -EOPNOTSUPP and
-EINVAL, but those errors are a bit more informative so we can
rationalize them a bit. Also we don't mark the server as broken based
on those errors (though maybe there's an argument to be made for doing
that).
Now however, you're adding one for -EIO. That error tends to be what we
get when an NT error code can't easily be translated to a POSIX error.
So now you might end up falling back and marking this server with
broken_qpath_info when the server might actually support those
infolevels just fine, but you hit some strange transient error.
I think it would be best to respin this set and add a new vers= value
(e.g. 1.0basic or something) that users can mount with to enable this
behavior. That way instead of bloating out the already horribly bloated
standard 1.0 codepaths, you could add a new set of smb_version ops and
have that enable the use of the more basic infolevels.
Yes, it means that users might have to read the manpage to figure this
out, but I think trying to do this automatically will prove quite
problematic.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
