This one applies to all versions of samba since registry shares have been introduced. A living session (smbd) will not notice changes in the registry shares. This is especially nasty when one needs to make a share unavailable and take it down for maintenance: Adding "available = no" to the share and issuing a "smbcontrol all reload-config" and a "smbcontrol all close-share SHARENAME" should disconnect all clients connected to the share and should prevent them from reconnecting. But for registry shares, in current code, the reconnection is not prevented.
Created attachment 6718 [details] Fix for v3-6-test This patchset, already pushed to master, fixes the issue for 3.6.
Created attachment 6719 [details] Add a couple of low-level commands to smbclient This patchset (picked from master) adds a couple of new commands to smbclient: logoff, tdis, tcon, tid (along with documentation), very useful for debugging. They can be used to reproduce the bug (and verify the fix) like this with smbclient: 1. define a share "share1", say, in registry and use registry config in smb.conf (e.g. include = registry) 2. connect to the share with smbclient: "smbclient //server/share1 -Uuser" and stay connected 3. mark the share unavailable on the samba server: "net conf setparm share1 available no" 4. reload the config: "smbcontrol all reload-config" 5. in the smbclient prompt, do "tdis" to close the share connection 6. in the smbclient prompt, do "tcon share1", to try to reconnect to the share Without the patch, step #6 will succeed (which is the bug). With the patch #6 correctly fails.
Comment on attachment 6719 [details] Add a couple of low-level commands to smbclient not sure if these innocious but useful additions could be still taken? (possibly for 3.6.1)
==> karolin
sorry, there was a bug in the smbclient patch. taking back the bug for a moment...
Created attachment 6722 [details] fixed version of the smbclient patch This fixes two bugs introduced by directly cherry-picking from master. The original patch used cli_set_tid and friends which are new to master. This is the diff between the old and the new patchset: ~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -94,8 +93,7 @@ + + talloc_free(sharename); + -+ d_printf("tcon to %s successful, tid: %u\n", sharename, -+ cli_state_get_tid(cli)); ++ d_printf("tcon to %s successful, tid: %u\n", sharename, cli->cnum); + return 0; +} + @@ -196,14 +193,13 @@ + char *tid_str; + + if (!next_token_talloc(ctx, &cmd_ptr, &tid_str, NULL)) { -+ if (cli_state_has_tcon(cli)) { -+ d_printf("current tid is %d\n", cli_state_get_tid(cli)); ++ if (cli->cnum != -1) { ++ d_printf("current tid is %d\n", (int)(cli->cnum)); + } else { + d_printf("no tcon currently\n"); + } + } else { -+ uint16_t tid = atoi(tid_str); -+ cli_state_set_tid(cli, tid); ++ cli->cnum = (uint16_t)atoi(tid_str); + } + + return 0; ~~~~~~~~~~~~~~~~~~~~~~~~
Comment on attachment 6722 [details] fixed version of the smbclient patch This generated a compile warning: client/client.c:4263: warning: comparison is always true due to limited range of data type Additionally, something seems to be broken with the list function: smb: \> tdis tdis successful smb: \> list 0: server=int001st001, share=abc
The server change I'm ok with. The client change should *NOT* go in 3.6.0. It's not needed and just destabilizes the tree. If you need it, target 3.6.1 instead. Jeremy
(In reply to comment #8) > The server change I'm ok with. The client change should *NOT* go in 3.6.0. > It's not needed and just destabilizes the tree. If you need it, target 3.6.1 > instead. Yeah, agreed. (I just thought it would be nice to have, but it is not needed in any way. The degeree of destabilization is minimal though.) Assigning to Karolin for the server change. It is an important one! (And I would also like to port it for 3.5.)
I agree it's minimal disturbance. But it's the perfect example of new functionality that should be targeted for 3.6.1, not 3.6.0. We need to be doing blocker fixes only in 3.6.0 final right now. Jeremy.
(In reply to comment #10) > I agree it's minimal disturbance. But it's the perfect example of new > functionality that should be targeted for 3.6.1, not 3.6.0. We need to be doing > blocker fixes only in 3.6.0 final right now. I'm perfectly ok with this. I was just proposing stuff, and this smbclient patch I mainly added to this bug report so that people can easily reproduce the bug and verify the fix. Didn't mean to step on anyone's toes! Cheers - Michael
(In reply to comment #9) > Assigning to Karolin for the server change. It is an important one! > (And I would also like to port it for 3.5.) Pushed the server patch to v3-6-test. Would it be possible to backport it for 3.5.11 (maybe on August 4)? If not, is it a showstopper? Re-assigning to Michael to a) backport the server patch and b) fix the smbclient patch. Thanks!
Created attachment 6733 [details] Patchset for 3.5 This is the patchset for 3.5. I have cherry-picked the patches from 3.6, built and tested that the fix works. Karo, do you need additional review for 3.5?
Comment on attachment 6733 [details] Patchset for 3.5 Not sure whether review is required. Requesting review from Volker again..
Uh, I just noticed that this is still open. * The actual bugfix in 3.6 * It is not in 3.5 yet (probably because I did not reassign to Karolin) * Not sure what to about the innocuous smbclient patch. Maybe we should just skip it. Or should I propose it (again) for the next 3.6 release. It is just for convenience in testing really. Jeremy, I would like to hear your opinion on this... Assigning to Karolin for picking the 3.5 bugfix patch. Michael
(In reply to comment #15) > Assigning to Karolin for picking the 3.5 bugfix patch. Pushed to v3-5-test, re-assigning to Jeremy.
let's skip the silly smbclient extensions for 3.6 and mark this done... :)