Linux 2.6.35/TIPC 2.0 ABI breaking changes [was: TIPC 2.0 and TIPC_SUB_SERVICE]
Leandro Lucarella
> Hi Peter,
> Adding the version as a sockopt is a very is a very good idea.
> But actually, you can find out the version, and get around the two-binary problem aready now:
>
> Check if TIPC_SUB_SERVICE is zero. If it is, the version is 2.0,
> otherwise it is pre-2.0, and you should use the TIPC_SUB_SERVICE with
> a value of 1.
> A little hackish, maybe, but it would solve your problem (which is
> temporary anyway) provided you can alter and recompile your code.
>
> And, since you are anyway altering the code, you should take the
> opportunity to add the necessary byte-order conversions, since
> TIPC-1.6/7 are forward compatible in that respect.
>
> The only reason to introduce what Luca is suggesting, compatiblity
> code in the driver itself, is that you wouldn't need recompile your
> old apps.
> lf that is an absolute requirement we could do that, but our initial
> assessment was that most people recompile their code when moving to
> a new kernel. We may be wrong there, of course.
I thought the kernel usually went to great lengths to ensure old
userspace binaries work with newer kernels.
Since this, as you mention, is not hard to do, and does not impose
a penalty on new code, why not do it?
> It is also uncertain whether David M would accept such a change, or if
> he would see it as a way of sneaking back a functionality he already
> threw out.
By the way, this doesn't work at all for the "bindings" I've done to the
D programming language. D is ABI compatible with C, but not source
compatible, so to add TIPC support to D, I had to write all the constant
values again, in a D source file[1].
This means D can't be used to write code that uses TIPC if you target
kernels >= 2.6.35 (unless they write their own bindings, or at least use
a custom TIPC_SUB_SERVICE constant). I could change the TIPC_SUB_SERVICE
value, but then people using older kernels will be screwed. Another
option is to use the version statement (kind of like C's #ifdef):
version (TIPC_2_0)
const TIPC_SUB_SERVICE = 0x00;
else
const TIPC_SUB_SERVICE = 0x02;
But then, *users* (I mean *end-users* not developers) will need to use
a compiler switch to select for which kernel they would like to compile
their applications, which I find unacceptable.
I think is really very harsh too to force people to recompile their
applications if they want to switch to a newer/older kernel to try
something out.
What I expected is to have something like 2 port names assigned to the
topology services instead of one, TIPC_TOP_SRV which is compatible with
TIPC 1 and marked as deprecated, and TIPC_TOP_SRV2 which only accepts
the TIPC 2.0 standard. Then, nothing of this would have happened.
And I think breaking changes should be discussed (or at least announced)
in this list more thoughtfully before including them in a Linux kernel
release. Is really a shame to be at a point where no matter what is
done, at least a couple of kernel releases will be binary incompatible
with old TIPC applications.
[1] http://www.dsource.org/projects/phobos/browser/trunk/phobos/std/c/linux/tipc.d
PS: Since this is David Miller's call, I'm Cc-ing him (and other kernel
mailing lists), as it makes no sense to speculate about what he
thinks if we can just ask him.
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
I'll take a quiet life,
a handshake of carbon monoxide,
with no alarms and no surprises,
no alarms and no surprises.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Leandro Lucarella
> This means D can't be used to write code that uses TIPC if you target
> kernels >= 2.6.35 (unless they write their own bindings, or at least use
> a custom TIPC_SUB_SERVICE constant). I could change the TIPC_SUB_SERVICE
> value, but then people using older kernels will be screwed. Another
> option is to use the version statement (kind of like C's #ifdef):
>
> version (TIPC_2_0)
> const TIPC_SUB_SERVICE = 0x00;
> else
> const TIPC_SUB_SERVICE = 0x02;
I keep finding problems with this change. Another, no D-related, problem
with this change is old code could be silently (and very subtly) broken
if they check for TIPC_SUB_SERVICE as a flag with something like:
if (s.filter & TIPC_SUB_SERVICE)
/* do something */
Because TIPC_SUB_SERVICE has changed its semantics, not just its value,
and the new value (0x00) will give you always 0 in that test. This one
is really tricky, because the application code will fail silently, there
will be no dmesg indication of a failure, nor the connection to the
topology service be closed. You might get a compiler warning if you're
lucky.
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
Novocaine for the soul
you better give me something
to fill the hole
before I sputter out
Paul Gortmaker
> Leandro Lucarella, el 18 de octubre a las 12:04 me escribiste:
>> This means D can't be used to write code that uses TIPC if you target
>> kernels >= 2.6.35 (unless they write their own bindings, or at least use
>> a custom TIPC_SUB_SERVICE constant). I could change the TIPC_SUB_SERVICE
>> value, but then people using older kernels will be screwed. Another
>> option is to use the version statement (kind of like C's #ifdef):
>>
>> � � � version (TIPC_2_0)
>> � � � � � � � const TIPC_SUB_SERVICE = 0x00;
>> � � � else
>> � � � � � � � const TIPC_SUB_SERVICE = 0x02;
>
> I keep finding problems with this change. Another, no D-related, problem
> with this change is old code could be silently (and very subtly) broken
> if they check for TIPC_SUB_SERVICE as a flag with something like:
>
> � � � �if (s.filter & TIPC_SUB_SERVICE)
> � � � � � � � �/* do something */
>
> Because TIPC_SUB_SERVICE has changed its semantics, not just its value,
> and the new value (0x00) will give you always 0 in that test. This one
> is really tricky, because the application code will fail silently, there
> will be no dmesg indication of a failure, nor the connection to the
> topology service be closed. You might get a compiler warning if you're
> lucky.
If you have access to the user space code in question, you can just
switch behaviour semantics based on the results of a uname call, knowing
that this change was included in versions since approx last Feb. There
is also /proc/version which can be parsed manually if you prefer.
If you have kernel code/modules, you can do the same/similar things with
the KERNEL_VERSION and its support macros.
In either case, you don't have to restrict yourself to information that is
specific to and/or exported just from TIPC itself.
Hope that helps,
Paul.
>
> --
> Leandro Lucarella (AKA luca) � � � � � � � � � � http://llucax.com.ar/
> ----------------------------------------------------------------------
> GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 �104C 949E BFB6 5F5A 8D05)
> ----------------------------------------------------------------------
> Novocaine for the soul
> you better give me something
> to fill the hole
> before I sputter out
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
David Miller
Date: Mon, 18 Oct 2010 16:42:33 -0400
> If you have access to the user space code in question, you can just
> switch behaviour semantics based on the results of a uname call, knowing
> that this change was included in versions since approx last Feb. There
> is also /proc/version which can be parsed manually if you prefer.
Requiring userspace to check kernel versioning information in order
to user an exported userspace API correctly is _ALWAYS_ _WRONG_.
You cannot and must not make backwards incompatible changes to
userspace interfaces.
Really, you can't.
Paul Gortmaker
> From: Paul Gortmaker<paul.go...@windriver.com>
> Date: Mon, 18 Oct 2010 16:42:33 -0400
>
>> If you have access to the user space code in question, you can just
>> switch behaviour semantics based on the results of a uname call, knowing
>> that this change was included in versions since approx last Feb. There
>> is also /proc/version which can be parsed manually if you prefer.
>
> Requiring userspace to check kernel versioning information in order
> to user an exported userspace API correctly is _ALWAYS_ _WRONG_.
>
> You cannot and must not make backwards incompatible changes to
> userspace interfaces.
What I think has happened here (and I'll double check this
tomorrow, since it is before I started assisting with tipc)
is that a backwards incompatible change *did* inadvertently
creep in via these two (related) commits:
--------------
commit d88dca79d3852a3623f606f781e013d61486828a
Author: Neil Horman <nho...@tuxdriver.com>
Date: Mon Mar 8 12:20:58 2010 -0800
tipc: fix endianness on tipc subscriber messages
--------------
and
---------------
commit c6537d6742985da1fbf12ae26cde6a096fd35b5c
Author: Jon Paul Maloy <jon....@ericsson.com>
Date: Tue Apr 6 11:40:52 2010 +0000
TIPC: Updated topology subscription protocol according to latest spec
---------------
Based on Leandro's info, I think it comes down to userspace
not knowing exactly where to find these bits anymore:
#define TIPC_SUB_SERVICE 0x00 /* Filter for service availability */
#define TIPC_SUB_PORTS 0x01 /* Filter for port availability */
#define TIPC_SUB_CANCEL 0x04 /* Cancel a subscription */
...because it doesn't know if there is the old auto endian
swap thing being done or not being done.
Assuming it is possible to do so in some non-kludgy way,
it sounds like we want to be looking into an in-kernel change
that ensures the older user space binaries get their
functionality restored then?
Thanks,
Paul.
Leandro Lucarella
Exactly. The damage is already done, the thing now is how to proceed.
What I'm doing right now is exactly that, use uname(2) to see what
kernel version is running and act according. Act according means, check
how with which tipc.h header the program was compiled (pre or post
2.6.35), compare against the current running kernel version, fix the BO
of subscriptions and fix the filter flags using my own constants
TIPC_SUB_SERVICE_V1 and TIPC_SUB_SERVICE_V2. Is not trivial, is not
nice.
And worse, as I said, for other languages bindings, specially those that
can't directly include a .h, there is no easy solution at all.
> Based on Leandro's info, I think it comes down to userspace
> not knowing exactly where to find these bits anymore:
>
> #define TIPC_SUB_SERVICE 0x00 /* Filter for service availability */
> #define TIPC_SUB_PORTS 0x01 /* Filter for port availability */
> #define TIPC_SUB_CANCEL 0x04 /* Cancel a subscription */
>
> ...because it doesn't know if there is the old auto endian
> swap thing being done or not being done.
There are, at least, 2 problems here. One is the auto endian swap (which
I knew it existed just because of this issue). But the auto endian swap
is not fully backward compatible either AFAIK, at least I tried Gentoo's
kernel 2.6.22 r5 and the topology services doesn't work with NBO (I
don't know if is a bug introduced by Gentoo, or a bug in the kernel or
what else). So just changing the BO and claiming backward compatibility
(even when is only at source code level) is not entirely true (unless is
a Gentoo issue of course, which I should probably check).
The other problem is TIPC_SUB_SERVICE changed it value from 2 to 0. In
TIPC 1.x it was a flag set in the filter member of a subscription. In
TIPC 2.0 is the absence of TIPC_SUB_PORTS. The change seems reasonable,
as TIPC_SUB_SERVICE and TIPC_SUB_PORTS were mutually exclusive and you
had to use one always, but the result is an API and *ABI* change. You
can't use an old TIPC application without changing the code and
recompiling using a tipc.h header from 2.6.35 or newer. And then, if you
need to reboot with an older kernel, the new compiled application won't
work with the old kernel.
A third problem, much less critical but which I find annoying, is that
an inconsistency was introduced. In kernels older than 2.6.35 (TIPC 1.6)
you used HBO for all the TIPC data structures, addresses, port names,
port sequences, port ids, subscriptions, events. Now, subscriptions and
events go in NBO (which contains port sequences and port ids), but when
binding a port name/sequence, you have to use HBO. Is true that
a subscription is supposed to be a network message and the port name you
bind to is not, but is at least inconsistent with AF_INET, where all you
do uses NBO.
> Assuming it is possible to do so in some non-kludgy way,
> it sounds like we want to be looking into an in-kernel change
> that ensures the older user space binaries get their
> functionality restored then?
I think the most reasonable way to do this was to use a different port
name for the topology service for TIPC 2.0, and keep the old TIPC 1.x
topology service at the same port name it was, and using the same
interface as it did. Adding new constants TIPC_TOP_SRV2 and
TIPC_SUB_SERVICE2 (for example) would have been enough to keep backward
compatibility and allow a new clean interface. The old topology service
could be deprecated and completely removed some time in a distant
future. You could even add some #ifdef TIPC_1_COMPATIBILITY to make sure
people using the old interface is aware of that and update to the new
one.
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
If you do not change your beliefs
Your life will always be like this
Neil Horman
tracked the endianess of the hosts to which it was connected and swapped data
that it sent to those hosts accordingly. With these changes the kernel client
simply swaps the data to network byte order on send and swaps it back to local
order on receive universally. That second commit added a bit from the reserved
pool of one of the connection establishment messages to indicate that a peer was
using this new protocol. If some non-local byte order information is making it
into user space, thats a bug that needs fixing.
What may be happening is some old client that doesn't know about the new bit
might be communicating with an new client that does. IIRC the spec called for
clients that set bits in the reserved field to drop frames from that client, so
that condition shouldn't occur, but TIPC may just be ignoring reserved bits. I
wouldn't be suprised.
Its also possible that the payload data between applications using tipc follow
the same broken byte swapping method that the protocol itself did, but if that
were the case I would expect the application to continue running normally,
unless user space had direct access to the protocol header in its entirety, and
read it directly, in which case I think I would just cry.
> ...because it doesn't know if there is the old auto endian
> swap thing being done or not being done.
>
> Assuming it is possible to do so in some non-kludgy way,
> it sounds like we want to be looking into an in-kernel change
> that ensures the older user space binaries get their
> functionality restored then?
>
Lets try figure out exactly what data is getting mis-read first. Maybe we can
fix it without having to go back to making a sending host figure out a receiving
hosts byte order. That would be nice. Can you describe the problem in more
detail?
Neil
Alan Cox
David Miller <da...@davemloft.net> wrote:
> From: Paul Gortmaker <paul.go...@windriver.com>
> Date: Mon, 18 Oct 2010 16:42:33 -0400
>
> > If you have access to the user space code in question, you can just
> > switch behaviour semantics based on the results of a uname call, knowing
> > that this change was included in versions since approx last Feb. There
> > is also /proc/version which can be parsed manually if you prefer.
>
> Requiring userspace to check kernel versioning information in order
> to user an exported userspace API correctly is _ALWAYS_ _WRONG_.
>
> You cannot and must not make backwards incompatible changes to
> userspace interfaces.
>
> Really, you can't.
In which case given that most distros are shipping older stuff and care
about ABI stability presumably the bug should get fixed for 2.6.36 ?
Leandro Lucarella
I think there is some misunderstanding here. The compatibility was
broken only for subscriptions messages. The subscriptions messages are
not sent between tipc clients (or maybe they are, but that's not how
tipc developers normally use them AFAIK). You send a subscription
message to your host tipc stack and the stack reply you with event
notifications. Even when they are message sent through a socket, they
are used as an API.
So, this has nothing to do with payload data transmitted by applications
using tipc. We are talking about the tipc API, which is "masked" into
a socket.
Here is a small example (~150 SLOC with comments). Using TIPC 2.0 API:
http://tipc.cslab.ericsson.net/cgi-bin/gitweb.cgi?p=people/allan/tipcutils.git;a=blob;h=efdfa3802e51d9a2a9091b3d97625de9e686b72e;hb=tipcutils2.0;f=demos/topology_subscr_demo/client_tipc.c
Using the "old" TIPC 1.6 API:
http://tipc.cslab.ericsson.net/cgi-bin/gitweb.cgi?p=people/allan/tipcutils.git;a=blob;h=ac5dfc5004b482372abb7905c90fe3073fc9165d;hb=15f57f7572898959e0aaa66293895a8255d77021;f=demos/topology_subscr_demo/subscriptions.c
> > ...because it doesn't know if there is the old auto endian
> > swap thing being done or not being done.
> >
> > Assuming it is possible to do so in some non-kludgy way,
> > it sounds like we want to be looking into an in-kernel change
> > that ensures the older user space binaries get their
> > functionality restored then?
> >
> Lets try figure out exactly what data is getting mis-read first. Maybe we can
> fix it without having to go back to making a sending host figure out a receiving
> hosts byte order. That would be nice. Can you describe the problem in more
> detail?
The problem is not between the tipc stacks in different hosts, is
between the tipc stack and the applications using it (well, maybe there
is a problem somewhere else too).
This was a deliberate API change, not a subtle bug...
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
For me to ask a woman out, I've got to get into a mental state like the karate
guys before they break the bricks.
-- George Constanza
David Miller
Date: Mon, 18 Oct 2010 23:16:57 -0300
>
> The problem is not between the tipc stacks in different hosts, is
> between the tipc stack and the applications using it (well, maybe
> there is a problem somewhere else too).
>
> This was a deliberate API change, not a subtle bug...
Neil et al., if these packets live only between the kernel stack
and the userspace API layer, we should not be byte-swapping this
stuff and we need to fix this fast.
Thanks.
David Miller
Date: Tue, 19 Oct 2010 07:04:52 -0400
> I'll have a patch together for Leandro to test soon.
Thank you.
Neil Horman
> From: Leandro Lucarella <lu...@llucax.com.ar>
> Date: Mon, 18 Oct 2010 23:16:57 -0300
>
> >
> > The problem is not between the tipc stacks in different hosts, is
> > between the tipc stack and the applications using it (well, maybe
> > there is a problem somewhere else too).
> >
> > This was a deliberate API change, not a subtle bug...
>
> Neil et al., if these packets live only between the kernel stack
> and the userspace API layer, we should not be byte-swapping this
> stuff and we need to fix this fast.
>
messages both off the wire and from local user space. The off the wire case
should work because the subscription code assumes that all the incomming data is
in network byte order, but user space is an exception to that rule as its in
local byte order. I'll have a patch together for Leandro to test soon.
Neil
Neil Horman
> Neil Horman, el 19 de octubre a las 07:04 me escribiste:
> > On Tue, Oct 19, 2010 at 01:16:49AM -0700, David Miller wrote:
> > > From: Leandro Lucarella <lu...@llucax.com.ar>
> > > Date: Mon, 18 Oct 2010 23:16:57 -0300
> > >
> > > >
> > > > The problem is not between the tipc stacks in different hosts, is
> > > > between the tipc stack and the applications using it (well, maybe
> > > > there is a problem somewhere else too).
> > > >
> > > > This was a deliberate API change, not a subtle bug...
> > >
> > > Neil et al., if these packets live only between the kernel stack
> > > and the userspace API layer, we should not be byte-swapping this
> > > stuff and we need to fix this fast.
> > >
> > Copy that Dave. I think I see the problem. The subscription code handles
> > messages both off the wire and from local user space. The off the wire case
> > should work because the subscription code assumes that all the incomming data is
> > in network byte order, but user space is an exception to that rule as its in
> > local byte order. I'll have a patch together for Leandro to test soon.
> > Neil
>
> problems, the other problem is the change in the value of
> TIPC_SUB_SERVICE from 2 to 0. That too is breaking the API/ABI, as
> a message with a filter value of 2 is rejected by TIPC 2.0/2.6.35+.
>
Yeah, that was the format change. We might just need to revert that. Lets see
about getting the endianess issue straight, and we'll go from there.
Neil
> --
> Leandro Lucarella (AKA luca) http://llucax.com.ar/
> ----------------------------------------------------------------------
> GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
> ----------------------------------------------------------------------
> Dentro de 30 a�os Argentina va a ser un gran supermercado con 15
> changuitos, porque esa va a ser la cantidad de gente que va a poder
> comprar algo.
> -- Sidharta Wiki
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
Leandro Lucarella
> > From: Leandro Lucarella <lu...@llucax.com.ar>
> > Date: Mon, 18 Oct 2010 23:16:57 -0300
> >
> > >
> > > The problem is not between the tipc stacks in different hosts, is
> > > between the tipc stack and the applications using it (well, maybe
> > > there is a problem somewhere else too).
> > >
> > > This was a deliberate API change, not a subtle bug...
> >
> > Neil et al., if these packets live only between the kernel stack
> > and the userspace API layer, we should not be byte-swapping this
> > stuff and we need to fix this fast.
> >
> Copy that Dave. I think I see the problem. The subscription code handles
> messages both off the wire and from local user space. The off the wire case
> should work because the subscription code assumes that all the incomming data is
> in network byte order, but user space is an exception to that rule as its in
> local byte order. I'll have a patch together for Leandro to test soon.
> Neil
Thank you very much. Bare in mind that the byte order is just one of the
problems, the other problem is the change in the value of
TIPC_SUB_SERVICE from 2 to 0. That too is breaking the API/ABI, as
a message with a filter value of 2 is rejected by TIPC 2.0/2.6.35+.
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
Dentro de 30 años Argentina va a ser un gran supermercado con 15
changuitos, porque esa va a ser la cantidad de gente que va a poder
comprar algo.
-- Sidharta Wiki
Neil Horman
> changuitos, porque esa va a ser la cantidad de gente que va a poder
> comprar algo.
> -- Sidharta Wiki
>
Hey all-
Heres what I have so far. Dave as a heads up please don't apply this
yet. I'd like to go over it a bit more and be sure of the implications here
before I post it for inclusion officially. I wanted Leandro to have a copy
though so he could confirm functionality for us. Leandro, This patch lets me
pass the tipc test code for TIPC 1.6 that you posted earlier this morning. If
you could confirm that it works for you that would be grand. While your doing
that, I want to read over the spec for TIPC and make sure that I'm not breaking
anything new with this patch.
Thanks!
Neil
diff --git a/include/linux/tipc.h b/include/linux/tipc.h
index 181c8d0..d8de884 100644
--- a/include/linux/tipc.h
+++ b/include/linux/tipc.h
@@ -127,9 +127,10 @@ static inline unsigned int tipc_node(__u32 addr)
* TIPC topology subscription service definitions
*/
-#define TIPC_SUB_SERVICE 0x00 /* Filter for service availability */
-#define TIPC_SUB_PORTS 0x01 /* Filter for port availability */
+#define TIPC_SUB_SERVICE 0x01 /* Filter for service availability */
+#define TIPC_SUB_PORTS 0x02 /* Filter for port availability */
#define TIPC_SUB_CANCEL 0x04 /* Cancel a subscription */
+#define TIPC_SUB_MASK (TIPC_SUB_SERVICE|TIPC_SUB_PORTS|TIPC_SUB_CANCEL)
#define TIPC_WAIT_FOREVER ~0 /* timeout for permanent subscription */
diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index 18813ac..06682e1 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -75,6 +75,17 @@ struct top_srv {
static struct top_srv topsrv = { 0 };
+/*
+ * Detect the need for an endian swap.
+ * user space sends subscriber info in
+ * host byte order while the kernel expects
+ * it in network byte order. To correct this
+ * lets check the filter bits, if there in the
+ * right place we know this is in network byte order
+ * otherwise it needs swapping around to maintain compatibility
+ */
+#define tipc_need_endian_swap(s) (!((s)->filter & TIPC_SUB_MASK))
+
/**
* subscr_send_event - send a message containing a tipc_event to the subscriber
*
@@ -279,11 +290,21 @@ static void subscr_cancel(struct tipc_subscr *s,
/* Find first matching subscription, exit if not found */
- type = ntohl(s->seq.type);
- lower = ntohl(s->seq.lower);
- upper = ntohl(s->seq.upper);
- timeout = ntohl(s->timeout);
- filter = ntohl(s->filter) & ~TIPC_SUB_CANCEL;
+ if (tipc_need_endian_swap(s)) {
+ printk(KERN_CRIT "Swapping endianess in subscr_cancel\n");
+ type = ntohl(s->seq.type);
+ lower = ntohl(s->seq.lower);
+ upper = ntohl(s->seq.upper);
+ timeout = ntohl(s->timeout);
+ filter = ntohl(s->filter) & ~TIPC_SUB_CANCEL;
+ } else {
+ printk(KERN_CRIT "NOT Swapping endianess in subscr_cancel\n");
+ type = s->seq.type;
+ lower = s->seq.lower;
+ upper = s->seq.upper;
+ timeout = s->timeout;
+ filter = s->filter & ~TIPC_SUB_CANCEL;
+ }
list_for_each_entry_safe(sub, sub_temp, &subscriber->subscription_list,
subscription_list) {
@@ -325,10 +346,19 @@ static struct subscription *subscr_subscribe(struct tipc_subscr *s,
struct subscriber *subscriber)
{
struct subscription *sub;
+ __u32 filter;
/* Detect & process a subscription cancellation request */
- if (ntohl(s->filter) & TIPC_SUB_CANCEL) {
+ if (tipc_need_endian_swap(s)) {
+ printk(KERN_CRIT "Swapping endianess in subscr_subscribe\n");
+ filter = ntohl(s->filter);
+ } else {
+ printk(KERN_CRIT "NOT Swapping endianess in subscr_subscribe\n");
+ filter = s->filter;
+ }
+
+ if (filter & TIPC_SUB_CANCEL) {
subscr_cancel(s, subscriber);
return NULL;
}
@@ -353,11 +383,22 @@ static struct subscription *subscr_subscribe(struct tipc_subscr *s,
/* Initialize subscription object */
- sub->seq.type = ntohl(s->seq.type);
- sub->seq.lower = ntohl(s->seq.lower);
- sub->seq.upper = ntohl(s->seq.upper);
- sub->timeout = ntohl(s->timeout);
- sub->filter = ntohl(s->filter);
+ if (tipc_need_endian_swap(s)) {
+ printk(KERN_CRIT "Swapping endianess in subscr_subscribe\n");
+ sub->seq.type = ntohl(s->seq.type);
+ sub->seq.lower = ntohl(s->seq.lower);
+ sub->seq.upper = ntohl(s->seq.upper);
+ sub->timeout = ntohl(s->timeout);
+ sub->filter = ntohl(s->filter);
+ } else {
+ printk(KERN_CRIT "NOT Swapping endianess in subscr_subscribe\n");
+ sub->seq.type = s->seq.type;
+ sub->seq.lower = s->seq.lower;
+ sub->seq.upper = s->seq.upper;
+ sub->timeout = s->timeout;
+ sub->filter = s->filter;
+ }
+
if ((sub->filter && (sub->filter != TIPC_SUB_PORTS)) ||
(sub->seq.lower > sub->seq.upper)) {
warn("Subscription rejected, illegal request\n");
Leandro Lucarella
> Hey all-
> Heres what I have so far. Dave as a heads up please don't apply this
> yet. I'd like to go over it a bit more and be sure of the implications here
> before I post it for inclusion officially. I wanted Leandro to have a copy
> though so he could confirm functionality for us. Leandro, This patch lets me
> pass the tipc test code for TIPC 1.6 that you posted earlier this morning. If
> you could confirm that it works for you that would be grand. While your doing
> that, I want to read over the spec for TIPC and make sure that I'm not breaking
> anything new with this patch.
Thanks for the quick response. I didn't tried the patch yet, but I think
spotted an error:
> diff --git a/include/linux/tipc.h b/include/linux/tipc.h
> index 181c8d0..d8de884 100644
> --- a/include/linux/tipc.h
> +++ b/include/linux/tipc.h
> @@ -127,9 +127,10 @@ static inline unsigned int tipc_node(__u32 addr)
> * TIPC topology subscription service definitions
> */
>
> -#define TIPC_SUB_SERVICE 0x00 /* Filter for service availability */
> -#define TIPC_SUB_PORTS 0x01 /* Filter for port availability */
> +#define TIPC_SUB_SERVICE 0x01 /* Filter for service availability */
> +#define TIPC_SUB_PORTS 0x02 /* Filter for port availability */
> #define TIPC_SUB_CANCEL 0x04 /* Cancel a subscription */
> +#define TIPC_SUB_MASK (TIPC_SUB_SERVICE|TIPC_SUB_PORTS|TIPC_SUB_CANCEL)
>
> #define TIPC_WAIT_FOREVER ~0 /* timeout for permanent subscription */
>
The values of TIPC_SUB_SERVICE and TIPC_SUB_PORTS seem to be swapped
compared to the old (TIPC 1.6) values:
#define TIPC_SUB_PORTS 0x01 /* filter for port availability */
#define TIPC_SUB_SERVICE 0x02 /* filter for service availability */
You might missed this error when trying the code I posted earlier (which
BTW are the official TIPC demos) because the change of behaviour when
using TIPC_SUB_SERVICE and TIPC_SUB_PORTS is very subtle (the former
reports only the first published port name that matches, and the later
all the published ports that match).
I'll test the patch tomorrow morning (ART).
Thanks again.
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
Long you live and high you fly
And smiles you'll give and tears you'll cry
And all you touch and all you see
Is all your life will ever be.
Leandro Lucarella
Also, I'm surprised the TIPC 1.6 demo program worked for you, as I don't
see any translation from the old TIPC_SUB_SERVICE value (2) to the new
one (0, or the absence of the TIPC_SUB_PORTS flag). I guess if this
subscription should be sent through the wire and should comply with
TIPC 2.0, filter & 2 should be 0.
Also, without such translation, I don't see how TIPC_SUB_SERVICE doesn't
trigger a subscription rejection with this check:
if ((sub->filter && (sub->filter != TIPC_SUB_PORTS)) ||
(sub->seq.lower > sub->seq.upper)) {
warn("Subscription rejected, illegal request\n");
But if you tried the TIPC 1.6 demo and it worked, I guess I'm definitely
missing something (as I'm not familiar with TIPC code at all).
Anyway, I'll try the patch tomorrow morning and tell you how it went,
but I thought I should point out those things just in case.
Thanks.
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
Be nice to nerds
Chances are you'll end up working for one
Leandro Lucarella
> Heres what I have so far. Dave as a heads up please don't apply this
> yet. I'd like to go over it a bit more and be sure of the implications here
> before I post it for inclusion officially. I wanted Leandro to have a copy
> though so he could confirm functionality for us. Leandro, This patch lets me
> pass the tipc test code for TIPC 1.6 that you posted earlier this morning. If
> you could confirm that it works for you that would be grand. While your doing
> that, I want to read over the spec for TIPC and make sure that I'm not breaking
> anything new with this patch.
I tried the patch (swapping the values of TIPC_SUB_SERVICE and
TIPC_SUB_PORTS) based on 2.6.35.4 and it didn't worked. dmesg sais:
NOT Swapping endianess in subscr_subscribe
NOT Swapping endianess in subscr_subscribe
TIPC: Subscription rejected, illegal request
I tried with a binary compiled with an older tipc.h header, I didn't
tried to recompile it using the new tipc.h (on purpose as the thing that
should be fixed is backwards compatibility).
I've read the TIPC 2.0 specification[1] a little more, and as I see, the
subscription messages are not supposed to go through the wire[2].
8. Topology Service
TIPC provides a message-based mechanism for an application to
learn about the port names that are visible to its node. This is
achieved by communicating with a Topology Service that has
knowledge of the contents of the node's name table.
So, if the idea is to comply with TIPC 2.0, the topology service should
accept the new TIPC_SUB_SERVICE and TIPC_SUB_PORTS values (0 and
1 in NBO respectively), and all the fields in the subscr struct should
be filled in NBO too.
However, if the idea is to keep backwards compatibility too, HBO should
be accepted as well as the old TIPC_SUB_SERVICE and TIPC_SUB_PORTS
values (2 and 1 in HBO respectively).
The real problem is, we can't figure out the endianess of the subscr
struct because 0x0 is a valid filter in TIPC 2.0.
The only solution I see is to change the TIPC 2.0 specification (which
is a "work-in-progress") to make the topology service use the port name
{2,2}, leaving {1,1} for backwards compatibility. Then add the constants
TIPC_SUB_SERVICE2 (0) and TIPC_TOP_SRV2 (2), or similar, to use the TIPC
2.0 interface and leave TIPC_SUB_SERVICE and TIPC_TOP_SRV for the TIPC
1.x interface.
Another option is to change the TIPC 2.0 specification to use the old
format (use HBO in subscriptions and keep TIPC_SUB_SERVICE as a separate
flag with value 2) and forget about all this. After all, I can't see
what advantages gives having to change the BO for internal messages
between the applications and the stack.
[1] http://tipc.sourceforge.net/doc/draft-spec-tipc-06.html
[2] http://tipc.sourceforge.net/doc/draft-spec-tipc-06.html#anchor92
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
CARANCHO OBNUBILADO APARECE EN PARQUE CHACABUCO!
-- Crónica TV
Jon Maloy
> subscr struct because 0x0 is a valid filter in TIPC 2.0.
>
>
> Another option is to change the TIPC 2.0 specification to use
> the old format (use HBO in subscriptions and keep
> TIPC_SUB_SERVICE as a separate flag with value 2) and forget
> about all this. After all, I can't see what advantages gives
> having to change the BO for internal messages between the
> applications and the stack.
I agree with this. I have no problems with changing the draft
(which as Leandro already noted is "work-in-progress") to specify that
both HBO and NBO are permitted over the wire, and that it is the
topology server's task to keep track of which one is used.
Remember, permitting both is a superset of the current one (NBO only)
so it is fully backwards compatible. We break absolutly nothing by
permitting this.
>
> [1] http://tipc.sourceforge.net/doc/draft-spec-tipc-06.html
> [2] http://tipc.sourceforge.net/doc/draft-spec-tipc-06.html#anchor92
>
> --
> Leandro Lucarella (AKA luca) http://llucax.com.ar/
> ----------------------------------------------------------------------
> GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
> ----------------------------------------------------------------------
> CARANCHO OBNUBILADO APARECE EN PARQUE CHACABUCO!
> -- Cr�nica TV
> --
> To unsubscribe from this list: send the line "unsubscribe
> netdev" in the body of a message to majo...@vger.kernel.org
Neil Horman
> Neil Horman, el 19 de octubre a las 16:18 me escribiste:
> > Heres what I have so far. Dave as a heads up please don't apply this
> > yet. I'd like to go over it a bit more and be sure of the implications here
> > before I post it for inclusion officially. I wanted Leandro to have a copy
> > though so he could confirm functionality for us. Leandro, This patch lets me
> > pass the tipc test code for TIPC 1.6 that you posted earlier this morning. If
> > you could confirm that it works for you that would be grand. While your doing
> > that, I want to read over the spec for TIPC and make sure that I'm not breaking
> > anything new with this patch.
>
> I tried the patch (swapping the values of TIPC_SUB_SERVICE and
> TIPC_SUB_PORTS) based on 2.6.35.4 and it didn't worked. dmesg sais:
> NOT Swapping endianess in subscr_subscribe
> NOT Swapping endianess in subscr_subscribe
> TIPC: Subscription rejected, illegal request
set the flags properly in user space.
Ugh, the tipc 'spec' is just a mess (note section 2.4.2, 8.2.1, etc also indicates all
mesages should be in network byte order)
What we should probably do is, for the time being, just revert my endian swap
commit, plus pauls bit field change to get us back to a point where we're no
longer breaking user space. Then we can take our time to find a way to conform
to the spec in a backwards compatible manner. I'll send patches to do that
shortly.
Neil
> --
> Leandro Lucarella (AKA luca) http://llucax.com.ar/
> ----------------------------------------------------------------------
> GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
> ----------------------------------------------------------------------
> CARANCHO OBNUBILADO APARECE EN PARQUE CHACABUCO!
> -- Cr�nica TV
Neil Horman
> <...>
> > subscr struct because 0x0 is a valid filter in TIPC 2.0.
> >
>
> >
> > Another option is to change the TIPC 2.0 specification to use
> > the old format (use HBO in subscriptions and keep
> > TIPC_SUB_SERVICE as a separate flag with value 2) and forget
> > about all this. After all, I can't see what advantages gives
> > having to change the BO for internal messages between the
> > applications and the stack.
>
> I agree with this. I have no problems with changing the draft
> (which as Leandro already noted is "work-in-progress") to specify that
> both HBO and NBO are permitted over the wire, and that it is the
> topology server's task to keep track of which one is used.
>
> Remember, permitting both is a superset of the current one (NBO only)
> so it is fully backwards compatible. We break absolutly nothing by
> permitting this.
>
disagreeing with it, just looking for clarification). If we revert my patch and
reintroduce the htohl mechanism which tracks endianess, we might as well revert
the TIPC_SUB_SERVICE flag as well, yeah?
Neil
Jon Maloy
> >
> > Remember, permitting both is a superset of the current one
> (NBO only)
> > so it is fully backwards compatible. We break absolutly nothing by
> > permitting this.
> >
> Thats effectively reverting both our patches though, isn't it
> (not that I'm disagreeing with it, just looking for
> clarification). If we revert my patch and reintroduce the
> htohl mechanism which tracks endianess, we might as well
> revert the TIPC_SUB_SERVICE flag as well, yeah?
Absolutely. I think it was a mistake to change that value.
But I don't think we need to reintroduce the htohl(). That
was just one way of doing it. If I understood your suggestion
from yesterday correctly you converted the whole message within
one if()clause, without any htohl(). I have have no problem with
that approach.
///jon
>
> Neil
>
> >
> > >
> > > [1] http://tipc.sourceforge.net/doc/draft-spec-tipc-06.html
> > > [2]
> http://tipc.sourceforge.net/doc/draft-spec-tipc-06.html#anchor92
> > >
> > > --
> > > Leandro Lucarella (AKA luca)
> http://llucax.com.ar/
> > >
> --------------------------------------------------------------------
> > > -- GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E
> BFB6 5F5A
> > > 8D05)
> > >
> --------------------------------------------------------------------
> > > -- CARANCHO OBNUBILADO APARECE EN PARQUE CHACABUCO!
Leandro Lucarella
> > Another option is to change the TIPC 2.0 specification to use
> > the old format (use HBO in subscriptions and keep
> > TIPC_SUB_SERVICE as a separate flag with value 2) and forget
> > about all this. After all, I can't see what advantages gives
> > having to change the BO for internal messages between the
> > applications and the stack.
>
> I agree with this. I have no problems with changing the draft
> (which as Leandro already noted is "work-in-progress") to specify that
> both HBO and NBO are permitted over the wire, and that it is the
> topology server's task to keep track of which one is used.
Just to try to understand better how things works, or are supposed to
work: do the subscription and event messages (and I mean the struct
tipc_subscr and tipc_event published in tipc.h) really go over the wire
or are only used to communicate the stack to the application inside
a node?
I think this is a crucial matter, since it defines if the changes cross
kernel/userspace boundaries only or it also crosses the kernel/network
boundaries.
> Remember, permitting both is a superset of the current one (NBO only)
> so it is fully backwards compatible. We break absolutly nothing by
> permitting this.
I think if they really go through the wire, it should be in NBO, and if
tipc_subscr and tipc_event are used only internally, we can still fix
the userspace messages when sending them through the wire.
In any case, I agree that the patches should be reverted and a solution
should be planned with more time and consensus.
Thanks.
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
The world's best known word is "okay"
The second most well-known word is "Coca-Cola"
Leandro Lucarella
> <...>
> > >
> > > Remember, permitting both is a superset of the current one
> > (NBO only)
> > > so it is fully backwards compatible. We break absolutly nothing by
> > > permitting this.
> > >
> > Thats effectively reverting both our patches though, isn't it
> > (not that I'm disagreeing with it, just looking for
> > clarification). If we revert my patch and reintroduce the
> > htohl mechanism which tracks endianess, we might as well
> > revert the TIPC_SUB_SERVICE flag as well, yeah?
>
> Absolutely. I think it was a mistake to change that value.
> But I don't think we need to reintroduce the htohl(). That
> was just one way of doing it. If I understood your suggestion
> from yesterday correctly you converted the whole message within
> one if()clause, without any htohl(). I have have no problem with
> that approach.
There is a difference between both solutions, the htohl() version
tracked the need for swap as a struct subscription member (which was
used when sending back events). Neils patch doesn't do that tracking.
I don't really know the implications of this, but maybe it would be
a wise idea to stay in the safe side and revert both patches for now.
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
It's not a lie, if you believe it.
-- George Constanza
Jon Maloy
>
> Just to try to understand better how things works, or are supposed to
> work: do the subscription and event messages (and I mean the
> struct tipc_subscr and tipc_event published in tipc.h) really
> go over the wire or are only used to communicate the stack to
> the application inside a node?
Both. And, given TIPC fundamental "location transparency" principle
the sender (or receiver) at user level does not need to know the
difference.
For a TIPC user, all messages are "local", insofar they stay within
the same cluster.
>
> I think this is a crucial matter, since it defines if the
> changes cross kernel/userspace boundaries only or it also
> crosses the kernel/network boundaries.
>
> > Remember, permitting both is a superset of the current one
> (NBO only)
> > so it is fully backwards compatible. We break absolutly nothing by
> > permitting this.
>
> I think if they really go through the wire, it should be in
> NBO, and if tipc_subscr and tipc_event are used only
> internally, we can still fix the userspace messages when
> sending them through the wire.
There are plenty of protocols around not using NBO over the wire.
This is not a must.
Leandro Lucarella
> > I think if they really go through the wire, it should be in
> > NBO, and if tipc_subscr and tipc_event are used only
> > internally, we can still fix the userspace messages when
> > sending them through the wire.
>
> There are plenty of protocols around not using NBO over the wire.
> This is not a must.
Of course, but is harder to sniff and debug if you haven't a fixed BO,
so, if it's easy to adjust transparently to userspace, I think it could
worth the trouble.
But my main concern is backwards compatibility, everything else is
secondary :)
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
The Muppet show was banned from TV in Saudi Arabia
Because one of its stars was a pig
Leandro Lucarella
> Jon Maloy, el 20 de octubre a las 14:10 me escribiste:
> > <...>
> > > >
> > > > Remember, permitting both is a superset of the current one
> > > (NBO only)
> > > > so it is fully backwards compatible. We break absolutly nothing by
> > > > permitting this.
> > > >
> > > Thats effectively reverting both our patches though, isn't it
> > > (not that I'm disagreeing with it, just looking for
> > > clarification). If we revert my patch and reintroduce the
> > > htohl mechanism which tracks endianess, we might as well
> > > revert the TIPC_SUB_SERVICE flag as well, yeah?
> >
> > Absolutely. I think it was a mistake to change that value.
> > But I don't think we need to reintroduce the htohl(). That
> > was just one way of doing it. If I understood your suggestion
> > from yesterday correctly you converted the whole message within
> > one if()clause, without any htohl(). I have have no problem with
> > that approach.
>
> There is a difference between both solutions, the htohl() version
> tracked the need for swap as a struct subscription member (which was
> used when sending back events). Neils patch doesn't do that tracking.
> I don't really know the implications of this, but maybe it would be
> a wise idea to stay in the safe side and revert both patches for now.
BTW, I tried 2.6.37 reverting both offending patches and everything
seems to work well.
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
Vivimos en una época muy contemporánea, Don Inodoro...
-- Mendieta
Leandro Lucarella
I meant 2.6.35.7.
--
Leandro Lucarella (AKA luca) http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
Karma police
arrest this girl,
her Hitler hairdo
is making me feel ill
and we have crashed her party.