Bug 13499 - CTDB_BROADCAST_VNNMAP should not be used
Summary: CTDB_BROADCAST_VNNMAP should not be used
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: CTDB (show other bugs)
Version: 4.8.3
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-02 01:17 UTC by Martin Schwenke
Modified: 2020-12-11 08:22 UTC (History)
4 users (show)

See Also:

Attachments
Patch for 4.7 (11.36 KB, patch)
2018-07-06 01:57 UTC, Martin Schwenke 		amitay: review+
Details
Patch for v4.8 (12.96 KB, patch)
2018-07-06 01:57 UTC, Martin Schwenke 		amitay: review+
Details
Proposed additional patch for 4.7 to fix the build (1.44 KB, patch)
2018-07-06 10:49 UTC, Karolin Seeger 		martins: review+
amitay: review+
Details
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Schwenke 2018-07-02 01:17:32 UTC 
In both places where this is used a broadcast to all active nodes should be used instead.  In particular, traverses of volatile databases will not find records that have been updated on non-LMASTER nodes.

CTDB_BROADCAST_VNNMAP should simply be changed to CTDB_BROADCAST_ACTIVE and its implementation should be changed accordingly.
Comment 1 Martin Schwenke 2018-07-06 01:57:08 UTC 
Created attachment 14286 [details]
Patch for 4.7
Comment 2 Martin Schwenke 2018-07-06 01:57:54 UTC 
Created attachment 14287 [details]
Patch for v4.8
Comment 3 Amitay Isaacs 2018-07-06 08:15:42 UTC 
Hi Karolin,

This is ready for v4.7 and v4.8.
Comment 4 Karolin Seeger 2018-07-06 09:02:25 UTC 
(In reply to Amitay Isaacs from comment #3)
Pushed to autobuild-v4-[8,7]-test.
Comment 5 Karolin Seeger 2018-07-06 10:46:33 UTC 
Patch for 4.7 seems to be incomplete, breaks the build:

../source3/smbd/notifyd/notifyd.c: In function ‘notifyd_broadcast_reclog’:
../source3/smbd/notifyd/notifyd.c:1001:15: error: ‘CTDB_BROADCAST_VNNMAP’ undeclared (first use in this function)
   ctdbd_conn, CTDB_BROADCAST_VNNMAP,
               ^
../source3/smbd/notifyd/notifyd.c:1001:15: note: each undeclared identifier is reported only once for each function it appears in
Waf: Leaving directory `/memdisk/kseeger/a47/b661372/samba-ctdb/bin'
Build failed:  -> task failed (err #1): 
	{task: cc notifyd.c
Comment 6 Karolin Seeger 2018-07-06 10:49:45 UTC 
Created attachment 14307 [details]
Proposed additional patch for 4.7 to fix the build
Comment 7 Martin Schwenke 2018-07-06 11:42:16 UTC 
Hi Karolin,

Sorry I missed that.  I checked that there were no instances of CTDB_BROADCAST_VNNMAP in 4.8 but forgot to do that for 4.7  :-(

That looks sane.  The only complication is that in 4.8+ that particular instance has been changed to CTDB_BROADCAST_CONNECTED.

Your change is logically consistent and is more conservative, so I'm going to give it a +1.

That change from CTDB_BROADCAST_VNNMAP -> CTDB_BROADCAST_CONNECTED doesn't appear to have been considered a serious enough bug fix to need backporting, so we should be OK. Amitay is on CC: for this defect and I'll also add Volker to see if either of them disagree.
Comment 8 Martin Schwenke 2018-07-06 11:44:33 UTC 
Karolin, reassigning to you so you can move this along if you want to.  I'm going to bed.  :-)
Comment 9 Karolin Seeger 2018-07-09 10:55:20 UTC 
(In reply to Karolin Seeger from comment #6)
Pushed to autobuild-v4-7-test.
Comment 10 Karolin Seeger 2018-07-12 11:02:34 UTC 
Pushed to both branches.
Closing out bug report.

Thanks!