search

RestComm / jain-sip

Disclaimer: This repository is a git-svn mirror of the project found at http://java.net/projects/jsip whose original repository is developed collaboratively by the Advanced Networking Technologies Division at the National Institute of Standards and Technology (NIST) - an agency of the United States Department of Commerce and by a community of individual and enterprise contributors. TeleStax, Inc. will perform some productization work, new features experimentation branches, etc for its TelScale jSIP product that doesn't concern the community from the main repository hence this git repository.
http://www.restcomm.com/
141 stars 151 forks source link

#105 Jain-sip support IPV6address parsing for P Charging Vector header #113

Closed xhoaluu closed 7 years ago

xhoaluu commented 8 years ago

Hi @jaimecasero ,

I just update jain-sip to support IPV6address parsing for P charging vector header. Could you help me to review and give me comment.

jaimecasero commented 8 years ago

@xhoaluu have you rerun all parsing tests to check if there is some regresssion issue?

By the way, nice job on finding the key classes on you own, you rock!!

xhoaluu commented 8 years ago

hi @jaimecasero,

I have run "ant runtck" and the regression test is ok without any failing testcase. I will fix your comments and request new pull again :)

jaimecasero commented 8 years ago

@xhoaluu Please take my comments as open discussion, you are free to challenge my opinion. please dont hesitate here, and express your ideas, even if they are opposed to me. Im sure we will come to the best solution this way...

jaimecasero commented 8 years ago

@xhoaluu can you investigate the gov.nist.core.HostNameParser class regarding IPv6 parsing? . I wouldnt like to cause any duplication, im not sure we may reuse that existing logic for IMS headers...

xhoaluu commented 8 years ago

HI @jaimecasero,

Do you want to use HostNameParser to parse IPV6 address in IMS Headers to avoid duplicate code? Something likes I need to update tIpv6address() in LexerCore class:

public String tIpv6address() {
        int startIdx = ptr;
        try {
            String hostName = String.valueOf(buffer, startIdx, buffer.length - ptr - 1 );
            HostNameParser hnp = new HostNameParser(hostName);
            HostPort hp = hnp.hostPort(true);
            ptr = ptr + hp.getHost().hostname.length();
            return hp.getHost().hostname;
        } catch (ParseException ex) {
            return null;
        }
    }
jaimecasero commented 8 years ago

if possible it would be good to prevent any duplication. So i wanted to elaborate on reusing that class, yes. Seems you manage to find the way to reuse it, right? I guess the test passes the same, and your PR could be smaller...

xhoaluu commented 8 years ago

Yes, I will update the code and run tck again to make sure that I do not touch any thing on legacy, I will commit after that.

jaimecasero commented 8 years ago

where do we stand on this issue? Did you run the tests after all the modifications?

xhoaluu commented 8 years ago

Currently I just runtck test only. have not tried to test it under sip-servlet

jaimecasero commented 8 years ago

no need to run sip-servelet test for a jainSip modification, if you run JUnit test suite is enough

Do we we have positive restulst from test?

xhoaluu commented 8 years ago

Yes the test result is positive, all are green.

ant runtck result: BUILD SUCCESSFUL Total time: 21 minutes 3 seconds

jaimecasero commented 7 years ago

@xhoaluu TCK is just a set of the whole testsuite. Until we complete the mavenization of the project please run "ant cc-buildloop". That will invovle compilation and the whole testSuite

xhoaluu commented 7 years ago

@jaimecasero

after running "ant cc-buildloop", I saw that [junit] Test test.unit.gov.nist.javax.sip.parser.NioPipelineParserTest FAILED

but I run it manually, it's passed.

I have run the same regression on baseline jain-sip of restcomm, the result is the same.

jaimecasero commented 7 years ago

so far so good, some of the tests depend on timing and provide some false positive. Probably nothing to do with the new code/modifications. Merging