GNOME Bugzilla – Bug 793911
Show an error when smbc_opendir() doesn't set errno
Last modified: 2018-09-21 18:18:16 UTC
As mentioned in https://bugzilla.gnome.org/show_bug.cgi?id=793515#c6 if errno == 0, we should set a generic error rather than "success"
Created attachment 369101 [details] [review] smb: Work-around smbc_opendir() not setting errno "EIO" is the closest we can get to a relevant generic error.
Review of attachment 369101 [details] [review]: ::: daemon/gvfsbackendsmb.c @@ +1767,3 @@ + /* Work-around smbc_opendir() not setting errno */ + if (errsv == 0) I am not really sure that it can happen also here, it would need more investigation, but why not. However, you should also set errno = 0; before smbc_opendir. Maybe would be nice to create fixup_opendir_errno similar to fixup_open_errno, although it is only one occurrence... ::: daemon/gvfsbackendsmbbrowse.c @@ +922,3 @@ + /* Work-around smbc_opendir() not setting errno */ + if (errsv == 0) + errsv = EIO; We should also initialize errno to zero before... To be honest I am not sure this helps in some way. Yes, EIO is better than Success, but still, it doesn't explain why you can't connect. And you can't connect probably because SMB1 protocol is not used...
Created attachment 369108 [details] [review] smb: Work-around smbc_opendir() not setting errno "EIO" is the closest we can get to a relevant generic error.
Review of attachment 369108 [details] [review]: I don't see it reported upstream. I suppose that this misbehavior will be fixed together with the broken listing, but would be nice to report this upstream to be sure it won't be forgotten, can you please report it? I've just found the following report regarding another opendir misbehavior with recent samba version: https://bugzilla.samba.org/show_bug.cgi?id=13050 ::: daemon/gvfsbackendsmb.c @@ +1759,3 @@ smbc_stat = smbc_getFunctionStat (op_backend->smb_context); smbc_closedir = smbc_getFunctionClosedir (op_backend->smb_context); + Please remove the remaining whitespace completely... ::: daemon/gvfsbackendsmbbrowse.c @@ +917,3 @@ errsv, g_strerror (errsv)); + /* Work-around smbc_opendir() not setting errno */ + if (errsv == 0) Sorry, I haven't realized it before, but we should modify the errno only if dir == NULL. @@ +922,2 @@ if (dir == NULL && (op_backend->mount_cancelled || (errsv != EPERM && errsv != EACCES))) So maybe better to move it inside this if-statement...
Created attachment 369136 [details] [review] smbbrowse: Ask for credentials with recent samba behavior EEXISTS can be also returned with recent samba versions in case of invalid credentials, so the backend has to ask for credentials in this case also. See: https://bugzilla.samba.org/show_bug.cgi?id=13050
(In reply to Ondrej Holy from comment #4) > Review of attachment 369108 [details] [review] [review]: > > I don't see it reported upstream. I suppose that this misbehavior will be > fixed together with the broken listing, but would be nice to report this > upstream to be sure it won't be forgotten, can you please report it? I've > just found the following report regarding another opendir misbehavior with > recent samba version: > https://bugzilla.samba.org/show_bug.cgi?id=13050 You're the one that told me that smbc_opendir() could fail without setting an errno, in the bug mentioned in comment 0. I've never seen the problem, and I don't have any data to back it up. I'm not sure how I could report it upstream when I have zero data. (In reply to Ondrej Holy from comment #5) > Created attachment 369136 [details] [review] [review] > smbbrowse: Ask for credentials with recent samba behavior > > EEXISTS can be also returned with recent samba versions in case of > invalid credentials, so the backend has to ask for credentials in > this case also. See: > https://bugzilla.samba.org/show_bug.cgi?id=13050 And I don't see what this has to do with this particular bug.
(In reply to Bastien Nocera from comment #6) > ... > You're the one that told me that smbc_opendir() could fail without setting > an errno, in the bug mentioned in comment 0. I've never seen the problem, > and I don't have any data to back it up. I'm not sure how I could report it > upstream when I have zero data. I assumed that you are able to reproduce. Let's wait with it then, I will have to make more tests to see what's really happening...
Comment on attachment 369136 [details] [review] smbbrowse: Ask for credentials with recent samba behavior (In reply to Bastien Nocera from comment #6) > ... > And I don't see what this has to do with this particular bug. I found that upstream issue when I was looking corresponding upstream bug for this particular report and didn't want to forget about it. It is also a workaround, also for an opendir misbehavior, also fixing a wrong errno, also probably happening with Samba 4.7 only, so you can't say that it has nothing to do with this, but yes, I abused this bug and should have filed a new bug, but I was lazy... After some additional testing and investigation, I am convinced that it should not be needed for smbbrowse. So sorry for the spam, marking as obsolete.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gvfs/issues/328.