TIPC: Cluster Communication Protocol

Hi Leandro:

TIPC 2.0 requires that messages sent to the topology service have their
content stored in network byte order, unlike earlier versions of TIPC
which allowed the message content to be stored in either host byte order
or network byte order. Similarly, messages returned by the topology
service now store their content in network byte order only. If you
update your applications to use htonl() and ntohl() things should start
working again. You can take a look at the demos in the TIPC 2.0
utilities package for guidance in seeing how this is done; you can find
this code at:

http://tipc.cslab.ericsson.net/cgi-bin/gitweb.cgi?p=people/allan/tipcuti
ls.git;a=shortlog;h=tipcutils2.0 

Please be aware that this API change was mandated by the Linux
networking gatekeepers, rather than the TIPC Working Group. The Working
Group endeavours to maintain backwards compatability in TIPC's API, but
this change was beyond our control. We changed the version number of
TIPC to 2.0 to help highlight the fact that this version of TIPC is
distinct from previous versions, but we evidently need to do more to
publicize this change. I'll see what I can do to update the TIPC website
on SourceForge to make the distinctions between TIPC 2.0 and previous
versions clearer.

Regards,
Al 

> -----Original Message-----
> From: Leandro Lucarella [mailto:llucarella@...] 
> Sent: Monday, September 27, 2010 12:38 PM
> To: tipc-discussion@...
> Subject: [tipc-discussion] TIPC 2.0 and TIPC_SUB_SERVICE
> 
> Hi, I upgraded the Linux kernel to 2.6.35 and some 
> TIPC-enabled applications stopped working. I found the 
> problem and it's I get an illegal request error while using 
> TIPC_SUB_SERVICE subscriptions.
> I noted they are no longer supported in the current linux tree.
> 
> Is this correct or I'm missing something? Are you planning to 
> add them back or they are gone for ever? Any information on 
> what to expect in the future (about this and other breaking 
> changes) is highly appreciated.
> 
> Thank you.
> 
> --
> Leandro Lucarella
> Integratech S.A.
> 4571-5252
> 
> --------------------------------------------------------------
> ----------------
> Start uncovering the many advantages of virtual appliances 
> and start using them to simplify application deployment and 
> accelerate your shift to cloud computing.
> http://p.sf.net/sfu/novell-sfdev2dev
> _______________________________________________
> tipc-discussion mailing list
> tipc-discussion@...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

Stephens, Allan, el 29 de September a las 04:34 me escribiste:
> Hi Leandro:
> 
> TIPC 2.0 requires that messages sent to the topology service have their
> content stored in network byte order, unlike earlier versions of TIPC
> which allowed the message content to be stored in either host byte order
> or network byte order. Similarly, messages returned by the topology
> service now store their content in network byte order only. If you
> update your applications to use htonl() and ntohl() things should start
> working again. You can take a look at the demos in the TIPC 2.0
> utilities package for guidance in seeing how this is done; you can find
> this code at:
> 
> http://tipc.cslab.ericsson.net/cgi-bin/gitweb.cgi?p=people/allan/tipcuti
> ls.git;a=shortlog;h=tipcutils2.0 

Thanks, I was about to say that the demo package should be updated since
version 1.16 (the last one release) doesn't work with kernel 2.6.35. You
should probably do a release for the demos, since is very confusing for
users having a demo package that doesn't work.

I looked at the code and changes a little more and notice that
TIPC_SUB_SERVICE is still supported, but its value have changed, but the
distributions doesn't provide the new tipc.h, so there is no easy way to
compile a program that works with a newer kernel. The transition will be
extremely painful unfortunately.

About adding htonl() and ntohl(), I see a couple of problems too. What
about backwards compatibility? There is no (easy) to code an application
that work with both pre and post 2.6.35 kernels. And then, there is an
asymmetry between sockaddr_tipc and subscriptions/events, why
sockaddr_tipc should be filled in HBO while subscriptions in NBO. Y made
a small C++ wrapper for TIPC structures and this really makes things
much more complicated than they should be for me.

> Please be aware that this API change was mandated by the Linux
> networking gatekeepers, rather than the TIPC Working Group. The Working
> Group endeavours to maintain backwards compatability in TIPC's API, but
> this change was beyond our control. We changed the version number of
> TIPC to 2.0 to help highlight the fact that this version of TIPC is
> distinct from previous versions, but we evidently need to do more to
> publicize this change. I'll see what I can do to update the TIPC website
> on SourceForge to make the distinctions between TIPC 2.0 and previous
> versions clearer.

I'm really surprised about this (and a little disappointed to be
honest). It would be too hard to keep the previous API documenting it is
deprecated and add a new API using different a different mechanism (like
adding another subscription port name/instance to conncect to use the
new API).

Are you willing to accept patches with this changes?

I really think this is a very harmful approach, changing the API like
this in a public release, without any documentation about it, without
properly released examples, without any public discussion (at least on
this ML), and without an (easy) way to have code that can be binary (or
at least source) compatible with pre and post 2.6.35 kernels. Specially
when I see no point on changing the API at all.

Thank you.

-- 
Leandro Lucarella
Integratech S.A.
4571-5252

Leandro Lucarella, el 29 de septiembre a las 12:38 me escribiste:
> Stephens, Allan, el 29 de September a las 04:34 me escribiste:
> > Hi Leandro:
> > 
> > TIPC 2.0 requires that messages sent to the topology service have their
> > content stored in network byte order, unlike earlier versions of TIPC
> > which allowed the message content to be stored in either host byte order
> > or network byte order. Similarly, messages returned by the topology
> > service now store their content in network byte order only. If you
> > update your applications to use htonl() and ntohl() things should start
> > working again. You can take a look at the demos in the TIPC 2.0
> > utilities package for guidance in seeing how this is done; you can find
> > this code at:
> > 
> > http://tipc.cslab.ericsson.net/cgi-bin/gitweb.cgi?p=people/allan/tipcuti
> > ls.git;a=shortlog;h=tipcutils2.0 
> 
> Thanks, I was about to say that the demo package should be updated since
> version 1.16 (the last one release) doesn't work with kernel 2.6.35. You
> should probably do a release for the demos, since is very confusing for
> users having a demo package that doesn't work.

Shortly after sending this e-mail I understood that the demo package is
not integrated with the tools, maybe the web page should state that to
make it more clear.

-- 
Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
He andáu muchos caminos, muchos caminos he andáu, Chile tiene el buen
vino y Suecia, el bacalao. Esta'o Unido tiene el hot do', Cuba tiene el
mojito, Guatemala, el cornalito y Brasil la feishoada.

Leandro Lucarella, el 30 de septiembre a las 21:05 me escribiste:
> Leandro Lucarella, el 29 de septiembre a las 12:38 me escribiste:
> > Stephens, Allan, el 29 de September a las 04:34 me escribiste:
> > > Hi Leandro:
> > > 
> > > TIPC 2.0 requires that messages sent to the topology service have their
> > > content stored in network byte order, unlike earlier versions of TIPC
> > > which allowed the message content to be stored in either host byte order
> > > or network byte order. Similarly, messages returned by the topology
> > > service now store their content in network byte order only. If you
> > > update your applications to use htonl() and ntohl() things should start
> > > working again. You can take a look at the demos in the TIPC 2.0
> > > utilities package for guidance in seeing how this is done; you can find
> > > this code at:
> > > 
> > > http://tipc.cslab.ericsson.net/cgi-bin/gitweb.cgi?p=people/allan/tipcuti
> > > ls.git;a=shortlog;h=tipcutils2.0 
> > 
> > Thanks, I was about to say that the demo package should be updated since
> > version 1.16 (the last one release) doesn't work with kernel 2.6.35. You
> > should probably do a release for the demos, since is very confusing for
> > users having a demo package that doesn't work.
> 
> Shortly after sending this e-mail I understood that the demo package is
> not integrated with the tools, maybe the web page should state that to
  ^^^
Damn... s/not/now/

-- 
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 wrote: 

> > TIPC 2.0 requires that messages sent to the topology service have 
> > their content stored in network byte order, unlike earlier 
> versions of 
> > TIPC which allowed the message content to be stored in either host 
> > byte order or network byte order. Similarly, messages 
> returned by the 
> > topology service now store their content in network byte 
> order only. 
> > If you update your applications to use htonl() and ntohl() things 
> > should start working again. You can take a look at the demos in the 
> > TIPC 2.0 utilities package for guidance in seeing how this is done; 
> > you can find this code at:
> > 
> > 
> http://tipc.cslab.ericsson.net/cgi-bin/gitweb.cgi?p=people/allan/tipcu
> > ti
> > ls.git;a=shortlog;h=tipcutils2.0 

> I looked at the code and changes a little more and notice 
> that TIPC_SUB_SERVICE is still supported, but its value have 
> changed, but the distributions doesn't provide the new 
> tipc.h, so there is no easy way to compile a program that 
> works with a newer kernel. The transition will be extremely 
> painful unfortunately.
> 
> About adding htonl() and ntohl(), I see a couple of problems 
> too. What about backwards compatibility? There is no (easy) 
> to code an application that work with both pre and post 
> 2.6.35 kernels. And then, there is an asymmetry between 
> sockaddr_tipc and subscriptions/events, why sockaddr_tipc 
> should be filled in HBO while subscriptions in NBO. Y made a 
> small C++ wrapper for TIPC structures and this really makes 
> things much more complicated than they should be for me.

Even with this API change, it should still be possible to write a TIPC
application that works with both TIPC 2.0 and earlier versions. The
topology service present in the earlier versions of TIPC can detect
whether the subscribing application is using HBO or NBO, and will
respond in kind; in TIPC 2.0 the ability to process messages in HBO was
removed, and only NBO is supported there. This means that if you write
an application that uses NBO it should work for any version of TIPC.
(Caveat: It will still be necessary for the user to compile the
application using the appropriate version of tipc.h; that is, if you
compile an application using the TIPC 2.0 version of tipc.h it won't run
properly on a non-TIPC 2.0 system, and vice versa.)

Regards,
Al

PS. I see that Jon has already responded at length to the other points
in your email, so I won't bother repeating what he's already said on the
reasons for the change.

Stephens, Allan, el  1 de octubre a las 05:41 me escribiste:
> > About adding htonl() and ntohl(), I see a couple of problems 
> > too. What about backwards compatibility? There is no (easy) 
> > to code an application that work with both pre and post 
> > 2.6.35 kernels. And then, there is an asymmetry between 
> > sockaddr_tipc and subscriptions/events, why sockaddr_tipc 
> > should be filled in HBO while subscriptions in NBO. Y made a 
> > small C++ wrapper for TIPC structures and this really makes 
> > things much more complicated than they should be for me.
> 
> Even with this API change, it should still be possible to write a TIPC
> application that works with both TIPC 2.0 and earlier versions. The
> topology service present in the earlier versions of TIPC can detect
> whether the subscribing application is using HBO or NBO, and will
> respond in kind; in TIPC 2.0 the ability to process messages in HBO was
> removed, and only NBO is supported there. This means that if you write
> an application that uses NBO it should work for any version of TIPC.
> (Caveat: It will still be necessary for the user to compile the
> application using the appropriate version of tipc.h; that is, if you
> compile an application using the TIPC 2.0 version of tipc.h it won't run
> properly on a non-TIPC 2.0 system, and vice versa.)
> 
> Regards,
> Al
> 
> PS. I see that Jon has already responded at length to the other points
> in your email, so I won't bother repeating what he's already said on the
> reasons for the change.

OK, thanks for the patient explanation to both (BTW, Jon's e-mail didn't
went to the ML, I think it could be valuable for other users to have
that summary of the problems too).

I do remember the nasty discussions in March/April, I guess I didn't
followed close enough to see the API changes, I only remember the
discussion about the kernel upstream version not being the main one.

There are a few things that are still bugging me:

1. There is no way to add transparent binary and source compatibility?
   I mean, a patch for that wouldn't be accepter (either by you or the
   David Miller)? For example, assigning a new subscription port
   name/instance that accept subscription messages and send events in
   the old way?

2. Why the sockaddr values have to be in HBO and the subscription and
   event data in NBO? Is there any reason for that? Wouldn't be better
   to be consistent and have them all in NBO. It would be more
   consistent with how sockaddr_inet is handled.

Thanks.

-- 
Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
"Lidiar" no es lo mismo que "holguear"; ya que "lidiar" es relativo a
"lidia" y "holguear" es relativo a "olga".
	-- Ricardo Vaporeso

See below.
///jon

Jon Maloy M.Sc. EE
Researcher
Ericsson Canada
Broadband and Systems Research
8400 Decarie
H4P 2N2, Montreal, Quebec, Canada
Phone + 1 514 345-7900 x42056
Mobile + 1 514 591-5578
jon.maloy@...
 http://www.ericsson.com 
 

> -----Original Message-----
> From: Leandro Lucarella [mailto:luca@...] 
> Sent: October-01-10 11:01
> To: tipc-discussion@...
> Subject: Re: [tipc-discussion] TIPC 2.0 and TIPC_SUB_SERVICE
> 
> Stephens, Allan, el  1 de octubre a las 05:41 me escribiste:
> > > About adding htonl() and ntohl(), I see a couple of problems too. 
> > > What about backwards compatibility? There is no (easy) to code an 
> > > application that work with both pre and post
> > > 2.6.35 kernels. And then, there is an asymmetry between 
> > > sockaddr_tipc and subscriptions/events, why sockaddr_tipc 
> should be 
> > > filled in HBO while subscriptions in NBO. Y made a small 
> C++ wrapper 
> > > for TIPC structures and this really makes things much more 
> > > complicated than they should be for me.
> > 
> > Even with this API change, it should still be possible to 
> write a TIPC 
> > application that works with both TIPC 2.0 and earlier versions. The 
> > topology service present in the earlier versions of TIPC can detect 
> > whether the subscribing application is using HBO or NBO, and will 
> > respond in kind; in TIPC 2.0 the ability to process messages in HBO 
> > was removed, and only NBO is supported there. This means 
> that if you 
> > write an application that uses NBO it should work for any 
> version of TIPC.
> > (Caveat: It will still be necessary for the user to compile the 
> > application using the appropriate version of tipc.h; that 
> is, if you 
> > compile an application using the TIPC 2.0 version of tipc.h 
> it won't 
> > run properly on a non-TIPC 2.0 system, and vice versa.)
> > 
> > Regards,
> > Al
> > 
> > PS. I see that Jon has already responded at length to the 
> other points 
> > in your email, so I won't bother repeating what he's 
> already said on 
> > the reasons for the change.
> 
> OK, thanks for the patient explanation to both (BTW, Jon's 
> e-mail didn't went to the ML, I think it could be valuable 
> for other users to have that summary of the problems too).

That was intentional. 

> 
> I do remember the nasty discussions in March/April, I guess I 
> didn't followed close enough to see the API changes, I only 
> remember the discussion about the kernel upstream version not 
> being the main one.
> 
> There are a few things that are still bugging me:
> 
> 1. There is no way to add transparent binary and source compatibility?
>    I mean, a patch for that wouldn't be accepter (either by you or the
>    David Miller)? For example, assigning a new subscription port
>    name/instance that accept subscription messages and send events in
>    the old way?

We discussed this, but decided to take the hit, and not try to be backwards compatible.
Ok, if this becomes a BIG problem, we may have to reconsider, so far I don't have that impression.

> 
> 2. Why the sockaddr values have to be in HBO and the subscription and
>    event data in NBO? Is there any reason for that? Wouldn't be better
>    to be consistent and have them all in NBO. It would be more
>    consistent with how sockaddr_inet is handled.

The difference is that the subscription interface is a protocol, that may potentially go between nodes.
Originally, it was nod-internal, that is why we used HBO from the beginning.
The socket API is, and will always be node local, and represents an API, not a protocol. 

Anyway I think it is a bad idea to do any more changes to the API now for obvious reasons.


> 
> Thanks.
> 
> -- 
> Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
> ----------------------------------------------------------------------
> GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
> ----------------------------------------------------------------------
> "Lidiar" no es lo mismo que "holguear"; ya que "lidiar" es 
> relativo a "lidia" y "holguear" es relativo a "olga".
> 	-- Ricardo Vaporeso
> 
> 
> --------------------------------------------------------------
> ----------------
> Start uncovering the many advantages of virtual appliances 
> and start using them to simplify application deployment and 
> accelerate your shift to cloud computing.
> http://p.sf.net/sfu/novell-sfdev2dev
> _______________________________________________
> tipc-discussion mailing list
> tipc-discussion@...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

Jon Maloy, el  1 de octubre a las 11:22 me escribiste:
> > 1. There is no way to add transparent binary and source compatibility?
> >    I mean, a patch for that wouldn't be accepter (either by you or the
> >    David Miller)? For example, assigning a new subscription port
> >    name/instance that accept subscription messages and send events in
> >    the old way?
> 
> We discussed this, but decided to take the hit, and not try to be
> backwards compatible.  Ok, if this becomes a BIG problem, we may have
> to reconsider, so far I don't have that impression.

Just to know how you weight the size of the problem, your impression is
that is not a BIG problem because not many people brought it up?

Because for me is kind of painful, to install a system that is based on
TIPC, we have a pre-compiled image, that should run with different
kernels. Before this problem we didn't have to care much about the
kernel, now we have to build different binaries for different kernels
and that is a maintenance problem. I'm providing this information only
to help you to weight the problem, I don't know if you are considering
this issues when you do it.

> > 2. Why the sockaddr values have to be in HBO and the subscription and
> >    event data in NBO? Is there any reason for that? Wouldn't be better
> >    to be consistent and have them all in NBO. It would be more
> >    consistent with how sockaddr_inet is handled.
> 
> The difference is that the subscription interface is a protocol, that
> may potentially go between nodes.  Originally, it was nod-internal,
> that is why we used HBO from the beginning.  The socket API is, and
> will always be node local, and represents an API, not a protocol. 

I understand that, but is the same with sockaddr_inet, and it's members
are in NBO. I can't think of any reason for sockaddr_tipc not doing the
same, except for backwards compatibility, but it's already gone, so that
being out of the table, what is the reason to keep the inconsistency?

> Anyway I think it is a bad idea to do any more changes to the API now
> for obvious reasons.

I don't agree, at this point, there is no difference, you already broke
backwards compatibility, there isn't a better moment to fix this
inconsistency. Among all the BO changes one would have to make to be 2.0
compatible, I guess changing the sockaddrs too is not much painful.

Seriously, I think things are they are now, have the worse of both
worlds, you have broken backward compatibility and introduced
inconsistencies to the API.

Just as a side note, this new inconsistency is very painful for me
because I have a thin C++ wrapper for TIPC structures (for the BSD
sockets API in general), which automatically handle the BO issues. Now
I have a Name and NameSeq objects that can be used both for constructing
a SockAddrTIPC and a Subscription. With this change, I have to handle BO
differently in SockAddrTIPC and Subscription, because I can't abstract
the BO issues directly in Name and NameSeq because of the new
inconsistency.  This problem doesn't exist with INET address family,
I can abstract BO in the InAddr structure, and use it to construct
a SockAddrIn without any problems.


I hope you can reconsider at least one of the two issues I mentioned.

Thanks.

-- 
Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
Agarré, me fuí pal monte
Para ver el horizonte...

Hi Luca,
I agree that 1) is doable. It wouldn't really break the API. It is only a matter of having it done. 
Number 2) is out of the question, I would say. 
The API has been broken once, and some, if not most, people have taken the hit.
To break it again would be disastrous.

But, I'm not the one doing the work, so others should have the last word here.

Regards
///jon

Jon Maloy M.Sc. EE
Researcher
Ericsson Canada
Broadband and Systems Research
8400 Decarie
H4P 2N2, Montreal, Quebec, Canada
Phone + 1 514 345-7900 x42056
Mobile + 1 514 591-5578
jon.maloy@...
 http://www.ericsson.com 
 

> -----Original Message-----
> From: Leandro Lucarella [mailto:luca@...] 
> Sent: October-01-10 12:24
> To: tipc-discussion@...
> Subject: Re: [tipc-discussion] TIPC 2.0 and TIPC_SUB_SERVICE
> 
> Jon Maloy, el  1 de octubre a las 11:22 me escribiste:
> > > 1. There is no way to add transparent binary and source 
> compatibility?
> > >    I mean, a patch for that wouldn't be accepter (either 
> by you or the
> > >    David Miller)? For example, assigning a new subscription port
> > >    name/instance that accept subscription messages and 
> send events in
> > >    the old way?
> > 
> > We discussed this, but decided to take the hit, and not try to be 
> > backwards compatible.  Ok, if this becomes a BIG problem, 
> we may have 
> > to reconsider, so far I don't have that impression.
> 
> Just to know how you weight the size of the problem, your 
> impression is that is not a BIG problem because not many 
> people brought it up?
> 
> Because for me is kind of painful, to install a system that 
> is based on TIPC, we have a pre-compiled image, that should 
> run with different kernels. Before this problem we didn't 
> have to care much about the kernel, now we have to build 
> different binaries for different kernels and that is a 
> maintenance problem. I'm providing this information only to 
> help you to weight the problem, I don't know if you are 
> considering this issues when you do it.
> 
> > > 2. Why the sockaddr values have to be in HBO and the 
> subscription and
> > >    event data in NBO? Is there any reason for that? 
> Wouldn't be better
> > >    to be consistent and have them all in NBO. It would be more
> > >    consistent with how sockaddr_inet is handled.
> > 
> > The difference is that the subscription interface is a 
> protocol, that 
> > may potentially go between nodes.  Originally, it was nod-internal, 
> > that is why we used HBO from the beginning.  The socket API is, and 
> > will always be node local, and represents an API, not a protocol.
> 
> I understand that, but is the same with sockaddr_inet, and 
> it's members are in NBO. I can't think of any reason for 
> sockaddr_tipc not doing the same, except for backwards 
> compatibility, but it's already gone, so that being out of 
> the table, what is the reason to keep the inconsistency?
> 
> > Anyway I think it is a bad idea to do any more changes to 
> the API now 
> > for obvious reasons.
> 
> I don't agree, at this point, there is no difference, you 
> already broke backwards compatibility, there isn't a better 
> moment to fix this inconsistency. Among all the BO changes 
> one would have to make to be 2.0 compatible, I guess changing 
> the sockaddrs too is not much painful.
> 
> Seriously, I think things are they are now, have the worse of 
> both worlds, you have broken backward compatibility and 
> introduced inconsistencies to the API.
> 
> Just as a side note, this new inconsistency is very painful 
> for me because I have a thin C++ wrapper for TIPC structures 
> (for the BSD sockets API in general), which automatically 
> handle the BO issues. Now I have a Name and NameSeq objects 
> that can be used both for constructing a SockAddrTIPC and a 
> Subscription. With this change, I have to handle BO 
> differently in SockAddrTIPC and Subscription, because I can't 
> abstract the BO issues directly in Name and NameSeq because 
> of the new inconsistency.  This problem doesn't exist with 
> INET address family, I can abstract BO in the InAddr 
> structure, and use it to construct a SockAddrIn without any problems.
> 
> 
> I hope you can reconsider at least one of the two issues I mentioned.
> 
> Thanks.
> 
> -- 
> Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
> ----------------------------------------------------------------------
> GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
> ----------------------------------------------------------------------
> Agarré, me fuí pal monte
> Para ver el horizonte...
> 
> 
> --------------------------------------------------------------
> ----------------
> Start uncovering the many advantages of virtual appliances 
> and start using them to simplify application deployment and 
> accelerate your shift to cloud computing.
> http://p.sf.net/sfu/novell-sfdev2dev
> _______________________________________________
> tipc-discussion mailing list
> tipc-discussion@...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

Leandro Lucarella написа:
> Jon Maloy, el  1 de octubre a las 11:22 me escribiste:
>   
>>> 1. There is no way to add transparent binary and source compatibility?
>>>    I mean, a patch for that wouldn't be accepter (either by you or the
>>>    David Miller)? For example, assigning a new subscription port
>>>    name/instance that accept subscription messages and send events in
>>>    the old way?
>>>       
>> We discussed this, but decided to take the hit, and not try to be
>> backwards compatible.  Ok, if this becomes a BIG problem, we may have
>> to reconsider, so far I don't have that impression.
>>     
>
> Just to know how you weight the size of the problem, your impression is
> that is not a BIG problem because not many people brought it up?
>
> Because for me is kind of painful, to install a system that is based on
> TIPC, we have a pre-compiled image, that should run with different
> kernels. Before this problem we didn't have to care much about the
> kernel, now we have to build different binaries for different kernels
> and that is a maintenance problem. I'm providing this information only
> to help you to weight the problem, I don't know if you are considering
> this issues when you do it.
>   
I'm in the same situation as Leandro and I don't plan to use TIPC 2.0 
for the foreseeable future exclusivley because of this issue.
I would suggest a sockopt or something for getting the current TIPC 
version or protocol capabilities/options or something like that. This 
seems to be the only correct way to allow for a single image operation 
in a mixed tipc ver network and for me this is a must for a cluster 
protocol.

Regards,
Peter.

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.
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.


Regards
///jon


Jon Maloy M.Sc. EE
Researcher
Ericsson Canada
Broadband and Systems Research
8400 Decarie
H4P 2N2, Montreal, Quebec, Canada
Phone + 1 514 345-7900 x42056
Mobile + 1 514 591-5578
jon.maloy@...
 http://www.ericsson.com 
 

> -----Original Message-----
> From: Peter Litov [mailto:xpl@...] 
> Sent: October-01-10 22:55
> To: Leandro Lucarella
> Cc: tipc-discussion@...
> Subject: Re: [tipc-discussion] TIPC 2.0 and TIPC_SUB_SERVICE
> 
> Leandro Lucarella написа:
> > Jon Maloy, el  1 de octubre a las 11:22 me escribiste:
> >   
> >>> 1. There is no way to add transparent binary and source 
> compatibility?
> >>>    I mean, a patch for that wouldn't be accepter (either 
> by you or the
> >>>    David Miller)? For example, assigning a new subscription port
> >>>    name/instance that accept subscription messages and 
> send events in
> >>>    the old way?
> >>>       
> >> We discussed this, but decided to take the hit, and not try to be 
> >> backwards compatible.  Ok, if this becomes a BIG problem, 
> we may have 
> >> to reconsider, so far I don't have that impression.
> >>     
> >
> > Just to know how you weight the size of the problem, your 
> impression 
> > is that is not a BIG problem because not many people brought it up?
> >
> > Because for me is kind of painful, to install a system that 
> is based 
> > on TIPC, we have a pre-compiled image, that should run with 
> different 
> > kernels. Before this problem we didn't have to care much about the 
> > kernel, now we have to build different binaries for 
> different kernels 
> > and that is a maintenance problem. I'm providing this 
> information only 
> > to help you to weight the problem, I don't know if you are 
> considering 
> > this issues when you do it.
> >   
> I'm in the same situation as Leandro and I don't plan to use 
> TIPC 2.0 for the foreseeable future exclusivley because of this issue.
> I would suggest a sockopt or something for getting the 
> current TIPC version or protocol capabilities/options or 
> something like that. This seems to be the only correct way to 
> allow for a single image operation in a mixed tipc ver 
> network and for me this is a must for a cluster protocol.
> 
> Regards,
> Peter.
> 
> --------------------------------------------------------------
> ----------------
> Start uncovering the many advantages of virtual appliances 
> and start using them to simplify application deployment and 
> accelerate your shift to cloud computing.
> http://p.sf.net/sfu/novell-sfdev2dev
> _______________________________________________
> tipc-discussion mailing list
> tipc-discussion@...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

Jon Maloy, el  4 de octubre a las 11:08 me escribiste:
> 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.

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.

-- 
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

On Mon, Oct 18, 2010 at 2:46 PM, Leandro Lucarella <luca@...> wrote:
> 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
> the body of a message to majordomo@...
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

On 10-10-18 06:17 PM, David Miller wrote:
> From: Paul Gortmaker<paul.gortmaker@...>
> 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 <nhorman@...>
Date:   Mon Mar 8 12:20:58 2010 -0800

    tipc: fix endianness on tipc subscriber messages
--------------

and

---------------
commit c6537d6742985da1fbf12ae26cde6a096fd35b5c
Author: Jon Paul Maloy <jon.maloy@...>
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.

Paul Gortmaker, el 18 de octubre a las 19:11 me escribiste:
> On 10-10-18 06:17 PM, David Miller wrote:
> > From: Paul Gortmaker<paul.gortmaker@...>
> > 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 <nhorman@...>
> Date:   Mon Mar 8 12:20:58 2010 -0800
> 
>     tipc: fix endianness on tipc subscriber messages
> --------------
> 
> and
> 
> ---------------
> commit c6537d6742985da1fbf12ae26cde6a096fd35b5c
> Author: Jon Paul Maloy <jon.maloy@...>
> Date:   Tue Apr 6 11:40:52 2010 +0000
> 
>     TIPC: Updated topology subscription protocol according to latest spec
> ---------------

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

On Mon, Oct 18, 2010 at 07:11:37PM -0400, Paul Gortmaker wrote:
> On 10-10-18 06:17 PM, David Miller wrote:
> > From: Paul Gortmaker<paul.gortmaker@...>
> > 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 <nhorman@...>
> Date:   Mon Mar 8 12:20:58 2010 -0800
> 
>     tipc: fix endianness on tipc subscriber messages
> --------------
> 
> and
> 
> ---------------
> commit c6537d6742985da1fbf12ae26cde6a096fd35b5c
> Author: Jon Paul Maloy <jon.maloy@...>
> 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         */
> 
That shouldn't be the case.  Prior to the above changes the tipc implementation
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

> Thanks,
> Paul.
>

Neil Horman, el 18 de octubre a las 19:45 me escribiste:
> > 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 <nhorman@...>
> > Date:   Mon Mar 8 12:20:58 2010 -0800
> > 
> >     tipc: fix endianness on tipc subscriber messages
> > --------------
> > 
> > and
> > 
> > ---------------
> > commit c6537d6742985da1fbf12ae26cde6a096fd35b5c
> > Author: Jon Paul Maloy <jon.maloy@...>
> > 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         */
> > 
> That shouldn't be the case.  Prior to the above changes the tipc implementation
> 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.

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

On Tue, Oct 19, 2010 at 01:16:49AM -0700, David Miller wrote:
> From: Leandro Lucarella <luca@...>
> 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

> Thanks.
>

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 <luca@...>
> > 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

On Tue, Oct 19, 2010 at 10:19:36AM -0300, Leandro Lucarella wrote:
> 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 <luca@...>
> > > 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+.
> 
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
> the body of a message to majordomo@...
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

On Tue, Oct 19, 2010 at 10:19:36AM -0300, Leandro Lucarella wrote:
> 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 <luca@...>
> > > 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
> 


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");

Neil Horman, el 19 de octubre a las 16:18 me escribiste:
> 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, el 19 de octubre a las 17:43 me escribiste:
> Neil Horman, el 19 de octubre a las 16:18 me escribiste:
> > 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).

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

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

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

<...>
> 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 majordomo@... 
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

On Wed, Oct 20, 2010 at 02:20:18PM -0300, Leandro Lucarella wrote:
> 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
thats odd, it worked fine for me.  I wonder If I had an older tipc.h header that
set the flags properly in user space.

> 
> 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
> 
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
>

On Wed, Oct 20, 2010 at 01:57:06PM -0400, Jon Maloy wrote:
> <...>
> > 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. 
> 
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?

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!
> > 	-- Crónica TV
> > --
> > To unsubscribe from this list: send the line "unsubscribe 
> > netdev" in the body of a message to majordomo@... 
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

<...>
> > 
> > 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!
> > > 	-- Crónica TV
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe 
> netdev" in 
> > > the body of a message to majordomo@... More majordomo 
> > > info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > To unsubscribe from this list: send the line "unsubscribe 
> netdev" in 
> > the body of a message to majordomo@... More 
> majordomo info 
> > at  http://vger.kernel.org/majordomo-info.html
> > 
>

Jon Maloy, el 20 de octubre a las 13:57 me escribiste:
> > 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"

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.

-- 
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

<...>

> 
> 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. 

> 
> 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"
>

Jon Maloy, el 20 de octubre a las 14:37 me escribiste:
> > 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, el 20 de octubre a las 15:28 me escribiste:
> 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, el 20 de octubre a las 16:23 me escribiste:
> Leandro Lucarella, el 20 de octubre a las 15:28 me escribiste:
> > 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.

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.