[PATCH] Remove duplicate protocol definitions

Amitay Isaacs amitay at gmail.com
Tue Nov 3 01:52:12 UTC 2015


On Tue, Nov 3, 2015 at 9:36 AM, Martin Schwenke <martin at meltin.net> wrote:

> On Tue, 3 Nov 2015 09:13:26 +1100, Martin Schwenke <martin at meltin.net>
> wrote:
>
> > On Mon, 2 Nov 2015 15:37:38 +1100, Amitay Isaacs <amitay at gmail.com>
> > wrote:
> >
> > > Hi,
> > >
> > > Recently I introduced a new protocol/ sub-directory in CTDB that
> abstracts
> > > all the protocol structures and marshaling routines.  The new
> > > protocol/protocol.h defines the existing CTDB protocol elements.  This
> is
> > > essentially duplicates many of the structures defined in
> ctdb_protocol.h.
> > >
> > > Here is a patch-set that gets rid of duplicate structure definitions.
> In
> > > order to do that, all the existing structures in ctdb_protocol.h are
> > > renamed to their new counterpart in protocol/protocol.h.
> > >
> > >
> > >
> https://git.samba.org/?p=amitay/samba.git;a=shortlog;h=refs/heads/ctdb-protocol
> > >
> > >   (Patchset is large and therefore not attached.)
> > >
> > > There are two cases where a structure from ctdb_protocol.h does not
> match
> > > the corresponding new definition from protocol/protocol.h.
> > >
> > > 1. The top level protocol structures (e.g. ctdb_req_call) in
> > > ctdb_protocol.h include the protocol header element.  Even though this
> > > makes the code easier, it creates structures with overlapping protocol
> > > elements.
> > >
> > > 2. Many of the control protocol structures include array elements.  The
> > > corresponding structure definitions in ctdb_protocol.h represent this
> array
> > > elements as they would appear on the wire.  This makes the coding
> > > error-prone (requiring the use of offsetof() to correctly calculate
> > > structure sizes).  The new definitions clearly mark those array
> elements as
> > > such and do not force the wire format.
> > >
> > > Both these types of structures are suffixed with "_old" to indicate
> > > remnants of old protocol implementation.
> > >
> > > Review/comments and push appreciated.
> >
> > Reviewed-by: Martin Schwenke <martin at meltin.net>
> >
> > Pushed...
>
> Fell over in autobuild.  It looks like building without cluster support
> doesn't work:
>
> [2904/4075] Compiling source3/smbd/notifyd/notifyd.c
> In file included from ../source3/smbd/notifyd/notifyd.c:36:
> ../ctdb/include/ctdb_protocol.h:24:31: error: protocol/protocol.h: No such
> file or directory
> In file included from ../source3/smbd/notifyd/notifyd.c:36:
> ../ctdb/include/ctdb_protocol.h:52: error: array type has incomplete
> element type
> ../ctdb/include/ctdb_protocol.h:62: error: field 'hdr' has incomplete type
> ../ctdb/include/ctdb_protocol.h:73: error: field 'hdr' has incomplete type
> ../ctdb/include/ctdb_protocol.h:80: error: field 'hdr' has incomplete type
> ../ctdb/include/ctdb_protocol.h:87: error: field 'hdr' has incomplete type
> ../ctdb/include/ctdb_protocol.h:97: error: field 'hdr' has incomplete type
> ../ctdb/include/ctdb_protocol.h:106: error: field 'hdr' has incomplete type
> ../ctdb/include/ctdb_protocol.h:113: error: field 'hdr' has incomplete type
> ../ctdb/include/ctdb_protocol.h:124: error: field 'hdr' has incomplete type
> ../ctdb/include/ctdb_protocol.h:132: error: field 'hdr' has incomplete type
> ../ctdb/include/ctdb_protocol.h:142: error: array type has incomplete
> element type
> ../ctdb/include/ctdb_protocol.h:147: error: array type has incomplete
> element type
> ../ctdb/include/ctdb_protocol.h:166: error: array type has incomplete
> element type
> ../ctdb/include/ctdb_protocol.h:178: error: field 'latency' has incomplete
> type
> ../ctdb/include/ctdb_protocol.h:179: error: 'MAX_COUNT_BUCKETS' undeclared
> here (not in a function)
> ../ctdb/include/ctdb_protocol.h:182: error: field 'latency' has incomplete
> type
> ../ctdb/include/ctdb_protocol.h:191: error: 'MAX_HOT_KEYS' undeclared here
> (not in a function)
> ../ctdb/include/ctdb_protocol.h:214: error: array type has incomplete
> element type
> ../ctdb/include/ctdb_protocol.h:219: error: array type has incomplete
> element type
> ../ctdb/include/ctdb_protocol.h:224: error: expected
> specifier-qualifier-list before 'ctdb_sock_addr'
> ../ctdb/include/ctdb_protocol.h:234: error: expected
> specifier-qualifier-list before 'ctdb_sock_addr'
> ../ctdb/include/ctdb_protocol.h:277: error: field 'ip' has incomplete type
> ../ctdb/include/ctdb_protocol.h:280: error: array type has incomplete
> element type
> ../ctdb/include/ctdb_protocol.h:285: error: array type has incomplete
> element type
> ../ctdb/include/ctdb_protocol.h:293: error: field 'hdr' has incomplete type
> ../source3/smbd/notifyd/notifyd.c: In function 'notifyd_broadcast_reclog':
> ../source3/smbd/notifyd/notifyd.c:975: error: 'CTDB_BROADCAST_VNNMAP'
> undeclared (first use in this function)
> ../source3/smbd/notifyd/notifyd.c:975: error: (Each undeclared identifier
> is reported only once
> ../source3/smbd/notifyd/notifyd.c:975: error: for each function it appears
> in.)
> Waf: Leaving directory `/memdisk/martins/a/b25239/samba/bin'
> Build failed:  -> task failed (err #1):
>         {task: cc notifyd.c -> notifyd_1.o}
> make: *** [all] Error 1
>
> :-(
>
>
The problem is that source3/smbd/server.c and
source3/smbd/notifyd/notifyd.c include CTDB headers without checking for
CLUSTER_SUPPORT.  There are two ways to fix this:

1. Always check for CLUSTER_SUPPORT before including CTDB headers.

Additional patches to fix the above two files attached.  These patches add
#ifdef CLUSTER_SUPPORT checks to the above mentioned files.

2. If we need to build smbd with clustering, but restrict the functionality
at runtime using lp_clustering(), then always build with CTDB and drop the
--with-cluster-support configure option.

Volker, if we prefer option 1 then can you please review the patches?

Amitay.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ctdb.patches
Type: application/octet-stream
Size: 7141 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151103/c8b683bd/ctdb.obj>


More information about the samba-technical mailing list