On Sat, Apr 17, 2010 at 08:22:16PM +0200, Andrew Beekhof wrote:
> Sorry, pressed send too quickly...
>
> On Sat, Apr 17, 2010 at 8:13 PM, Lars Ellenberg
> <[email protected]> wrote:
> > On Sat, Apr 17, 2010 at 07:58:38PM +0200, Andrew Beekhof wrote:
> >> I vote for reapplying the patch, bumping the SO name and forgetting
> >> about the whole thing.
> >
> > The only thing I do is move the two new members to the end of the struct,
> > keeping backwards compatibility, before bumping the SO name anyways,
> > though not from 2.0.0 to 3.0.0, but only to 2.1.0.
> >
> > Because I don't see why we would insist on breaking backwards
> > compatibility, if keeping it is that cheap.
>
> I'll admit to not following the conversation that closely, but the
> revised patch you posted didn't look all that cheap to me.
> Or is that a different conversation?
No, that's it.
This is the interdiff:
Bump the so version, note this is the libtool
"interface:revision:age" way, the resulting soname
is libplumb.so.2.1.0
--- a/lib/clplumbing/Makefile.am Thu Apr 15 15:58:50 2010 +0200
+++ b/lib/clplumbing/Makefile.am Sat Apr 17 20:31:18 2010 +0200
@@ -63,7 +63,7 @@
libplumb_la_LIBADD = $(top_builddir)/replace/libreplace.la \
$(top_builddir)/lib/pils/libpils.la
-libplumb_la_LDFLAGS = -version-info 2:0:0
+libplumb_la_LDFLAGS = -version-info 3:0:1
libplumbgpl_la_SOURCES = setproctitle.c
libplumbgpl_la_LIBADD = $(top_builddir)/replace/libreplace.la \
move the new members to the end of the struct,
comment on their introduction purpose and time.
diff -u b/include/clplumbing/ipc.h b/include/clplumbing/ipc.h
--- b/include/clplumbing/ipc.h Sat Apr 17 20:32:19 2010 +0200
+++ b/include/clplumbing/ipc.h Sat Apr 17 20:31:18 2010 +0200
@@ -132,8 +132,6 @@
int ch_status; /* identify the status of channel.*/
int refcount; /* reference count */
pid_t farside_pid; /* far side pid */
- uid_t farside_uid; /* far side uid */
- gid_t farside_gid; /* far side gid */
void* ch_private; /* channel private data. */
/* (may contain conn. info.) */
IPC_Ops* ops; /* IPC_Channel function table.*/
@@ -180,6 +178,18 @@
int conntype;
char failreason[MAXFAILREASON];
+
+ /* New members to support Multi-level ACLs for the CIB,
+ * available since libplumb.so.2.1.0, added at the
+ * end of the struct to maintain backwards ABI compatibility.
+ *
+ * If you don't like to care for library versions,
+ * create your IPC channels with
+ * c = ipc_wait_conn_constructor(IPC_UDS_CRED, ...),
+ * and these members will be available.
+ */
+ uid_t farside_uid; /* far side uid */
+ gid_t farside_gid; /* far side gid */
};
struct IPC_QUEUE{
We can stop right here.
We (lge and dejanm) thought about doing this for glue 1.0.5,
but decided to first revert the thing, get the release done,
and put it back in the week after.
What I personally thought would be a good idea,
for people trying to talk with older glue as well as newer glue,
using the feature if available, but working without, if not,
is an additional alias to the ipc channel type.
Two comment chunks, the define itself, and one additional strcmp() each
at the client and server side of the channel constructors.
If that is stupid, and there is and never will be a user of this,
we drop that.
--- b/include/clplumbing/ipc.h Sat Apr 17 20:32:19 2010 +0200
+++ b/include/clplumbing/ipc.h Sat Apr 17 20:31:18 2010 +0200
@@ -614,8 +624,13 @@
* the pointer to a new waiting connection or NULL if the connection
* can't be created.
* Note:
- * current implementation only supports unix domain socket
- * whose type is IPC_DOMAIN_SOCKET
+ * current implementation supports
+ * IPC_ANYTYPE: alias for IPC_DOMAIN_SOCKET
+ * IPC_DOMAIN_SOCKET: unix domain sockets
+ * IPC_UDS_CRED: an other alias for unix domain sockets,
+ * available since libplumb.so.2.1.0.
+ * Using it, you can be sure that support for
+ * farside uid + gid credentials is available.
*
*/
extern IPC_WaitConnection * ipc_wait_conn_constructor(const char * ch_type
@@ -639,9 +654,8 @@
* or NULL if the channel can't be created.
*
* Note:
- * current implementation only support unix domain socket
- * whose type is IPC_DOMAIN_SOCKET
- *
+ * See comments for ipc_wait_conn_constructor above
+ * for currently implemented ch_type channel types.
*/
extern IPC_Channel * ipc_channel_constructor(const char * ch_type
, GHashTable* ch_attrs);
@@ -753,6 +767,9 @@
#define IPC_PATH_ATTR "path" /* pathname attribute */
#define IPC_DOMAIN_SOCKET "uds" /* Unix domain socket */
+/* Unix domain socket with farside uid + gid credentials.
+ * Available since libplumb.so.2.1.0 */
+#define IPC_UDS_CRED "uds_c"
#define IPC_MODE_ATTR "sockmode" /* socket mode attribute */
#ifdef IPC_DOMAIN_SOCKET
--- a/lib/clplumbing/ocf_ipc.c Thu Apr 15 15:58:50 2010 +0200
+++ b/lib/clplumbing/ocf_ipc.c Sat Apr 17 20:31:18 2010 +0200
@@ -62,6 +62,7 @@
ipc_wait_conn_constructor(const char * ch_type, GHashTable* ch_attrs)
{
if (strcmp(ch_type, "domain_socket") == 0
+ || strcmp(ch_type, IPC_UDS_CRED) == 0
|| strcmp(ch_type, IPC_ANYTYPE) == 0
|| strcmp(ch_type, IPC_DOMAIN_SOCKET) == 0) {
return socket_wait_conn_new(ch_attrs);
@@ -73,6 +74,7 @@
ipc_channel_constructor(const char * ch_type, GHashTable* ch_attrs)
{
if (strcmp(ch_type, "domain_socket") == 0
+ || strcmp(ch_type, IPC_UDS_CRED) == 0
|| strcmp(ch_type, IPC_ANYTYPE) == 0
|| strcmp(ch_type, IPC_DOMAIN_SOCKET) == 0) {
I try to summarize.
Introducing the new API in an ABI incompatible way without
reflecting that in the soname was a bug.
We all agree about that.
I commented on that earlier on the ML when the patch was posted.
I did not realize it was actually committed, otherwise I'd just
done the above right there and then. I thought it was an experimental
proof of concept, and would probably be solved differently anyways.
Dejan and me realized basically immediately once the announcement was
sent out that this bug was released. At that point it was unclear if
this newly introduced feature would be used by anyone at all.
Grepping though the clusterlabs hg repos did not show up anything.
We thought about possible ways to fix it, and decided -- to get rid of
the broken release out there -- to revert and release again _for now_,
then discuss if it was necessary at all, and put it back in in a
better way (and bumping the soname), if it was.
Turns out some want it desperately.
Fine, so we put it back in.
Meanwhile I had time to review and test various approaches,
and find that the first three chunks posted above make for
an equivalent API without breaking ABI backwards compatibility.
So why should we insist on breaking the ABI?
If the new IPC_UDS_CRED alias is usefull, well, I'm not sure.
I wanted to discuss that with Dejan next week face to face,
and committing the resulting modified patch back in.
There had been no need to get excited about it ;-)
What do we disagree about, again?
--
: Lars Ellenberg
: LINBIT | Your Way to High Availability
: DRBD/HA support and consulting http://www.linbit.com
DRBD® and LINBIT® are registered trademarks of LINBIT, Austria.
_______________________________________________________
Linux-HA-Dev: [email protected]
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/
