Bug 8327 - config reload fails to reload shares from registry
Summary: config reload fails to reload shares from registry
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: Config Files (show other bugs)
Version: 3.6.0rc3
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-27 06:55 UTC by Michael Adam
Modified: 2012-05-31 20:43 UTC (History)
4 users (show)

See Also:


Attachments
Fix for v3-6-test (2.25 KB, patch)
2011-07-27 06:57 UTC, Michael Adam
vl: review+
Details
Add a couple of low-level commands to smbclient (10.75 KB, patch)
2011-07-27 07:03 UTC, Michael Adam
vl: review+
Details
fixed version of the smbclient patch (10.56 KB, patch)
2011-07-27 12:17 UTC, Michael Adam
ambi: review-
Details
Patchset for 3.5 (2.33 KB, patch)
2011-07-29 22:16 UTC, Michael Adam
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Adam 2011-07-27 06:55:40 UTC
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.
Comment 1 Michael Adam 2011-07-27 06:57:27 UTC
Created attachment 6718 [details]
Fix for v3-6-test

This patchset, already pushed to master, fixes the issue for 3.6.
Comment 2 Michael Adam 2011-07-27 07:03:18 UTC
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 3 Michael Adam 2011-07-27 08:05:25 UTC
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)
Comment 4 Michael Adam 2011-07-27 09:49:48 UTC
==> karolin
Comment 5 Michael Adam 2011-07-27 11:54:18 UTC
sorry, there was a bug in the smbclient patch. taking back the bug for a moment...
Comment 6 Michael Adam 2011-07-27 12:17:40 UTC
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 7 Christian Ambach 2011-07-27 13:10:49 UTC
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
Comment 8 Jeremy Allison 2011-07-27 18:14:39 UTC
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
Comment 9 Michael Adam 2011-07-27 21:53:39 UTC
(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.)
Comment 10 Jeremy Allison 2011-07-27 21:55:22 UTC
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.
Comment 11 Michael Adam 2011-07-27 21:59:27 UTC
(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
Comment 12 Karolin Seeger 2011-07-29 19:09:39 UTC
(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!
Comment 13 Michael Adam 2011-07-29 22:16:21 UTC
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 14 Michael Adam 2011-07-29 22:17:35 UTC
Comment on attachment 6733 [details]
Patchset for 3.5

Not sure whether review is required. Requesting review from Volker again..
Comment 15 Michael Adam 2012-02-01 20:39:48 UTC
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
Comment 16 Karolin Seeger 2012-02-03 19:45:08 UTC
(In reply to comment #15)
> Assigning to Karolin for picking the 3.5 bugfix patch.

Pushed to v3-5-test, re-assigning to Jeremy.
Comment 17 Michael Adam 2012-05-31 20:43:07 UTC
let's skip the silly smbclient extensions for 3.6 and mark this done... :)