[Nice] Compatibility with MS Office Communicator 2007 R2

[Youness Alaoui]

On 10/01/2010 03:15 AM, Jakub Adam wrote:
> Hi Youness,
> 
>> Thanks, I re-reviewed them and applied them locally. However, you said that the
>> OC2007R2 compatibility should be made different from the WLM2009 one, but the
>> patch you gave me didn't have that.. it would mean another break of the API once
>> we integrate your TURN work... I suggest it would be made different right now
>> even if TURN support isn't integrated, so we don't break the API later.
> 
> Yes, I should split it before sending the last set of patches, sorry.
> There are no more changes to public API at agent/agent.h in TURN patch
> now.
> 

Thank you for quickly doing this! Good job! I reviewed the code, and *WOW*, this
is pretty much the first time I review code that I like and have no
comments/fixes to request!! Very good job, thanks!

Humm.. actually, I do have one small thing though, but the rest of the code is
very good and clean...
in priv_map_reply_to_relay_request, you added :
+            if (agent->compatibility == NICE_COMPATIBILITY_OC2007 ||
+                agent->compatibility == NICE_COMPATIBILITY_OC2007R2) {
+                  nice_turn_socket_set_ms_realm(relay_cand->sockptr,
&d->stun_message);
+                  nice_turn_socket_set_ms_connection_id(relay_cand->sockptr, resp);
+            }

I was wondering two things.. first, why use d->stun_message for set_ms_realm,
but use resp for set_ms_connection_id ? Secondly, why did you have to put this
here, why not add that code directly in the turn_socket_parse_recv directly and
remove those two functions from the turn_socket API ?
Is it because the realm is not set in the response, but it's only in the request
we send? in which case, we would already have the realm, so why set it again on
the turn_socket ? and if there's a reason for that, maybe that's why it's not in
parse_recv? because it needs information that the parse_recv doesn't get ?
Anyways, you either explain this to me with something that makes sense (in which
case, it would be good to add a comment there to explain why it's done like
that.. or you fix it by moving that code to the parse_recv and removing the
set_ms_* API from turn_socket...
In either case, I have already merged the 3 patches you sent into my local
libnice, so if you send a patch, send it based on top of the patches you already
sent, do not rebase or ammend your commits.

Thanks!


>> I do not expect the code to change
>>> much, you can review the current state of implementation and point out
>>> problems you see.
>> I think right now, i'll just wait for you to finish and clean the code before
>> reviewing it... it would be good to know though if the TURN support of OC2007R2
>> is also compatible with WLM2009's TURN servers... As I stated last time, the WLM
>> compatibility current uses the MSN TURN compat only because I didn't implement
>> the new WLM TURN support, and we were able to use the older MSN TURN servers...
> 
> After some tests I made yesterday, TURN is working well for me. Please
> consider attached version as final and ready for review.
> 
>> I see.. I can understand the API break for the
>> stun_message_validate_buffer_length, but I don't agree with what you added to
>> stun_agent_t.. I don't think anything should be added there.. if you need
>> something turn specific it should be added to the TurnPriv in socket/turn.c
> 
> I tried to address this problem by moving things from stun_agent_t to
> TurnPriv.
> 
>> Good.. I will wait for your update then, then make a release, but after tonight,
>> i'll be done with this quick libnice maintenance work as I have other stuff to
>> take care of. But I should be able to get some time to review your latest patch
>> and make the release.
> 
> Ok, I hope you'll find a couple of minutes to look at it soon.
> 
> Thanks,
> 
> Jakub
> 
> 
> 
> 
> _______________________________________________
> Nice mailing list
> Nice at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nice


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/nice/attachments/20101001/79224f80/attachment.pgp>