NDT-DEV email list created

[Thomas Gideon <>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 10/15/2012 04:30 PM, Dominic Hamon wrote:
> On Mon, Oct 15, 2012 at 1:25 PM, Kavitha Kumar
> <
>  
> <mailto:>>
>  wrote:
> 
> Hi Dominic,
> 
> Sorry about the delayed reply. My comments inline!
> 
> Thanks, Kavitha
> 
> ----- Original Message -----
>> From: "Dominic Hamon" 
>> <
> <mailto:>>
>> To: "Kavitha Kumar" 
>> <
> <mailto:>>
>> Cc: 
>> 
>>  
>> <mailto:>
>>  Sent:
>> Thursday, October 11, 2012 4:06:00 PM Subject: Re: [ndt-dev]
>> Submitting a patch
>> 
>> 
>> 
>> 
>> On Thu, Oct 11, 2012 at 11:35 AM, Kavitha Kumar < 
>> 
>>  
>> <mailto:>
>>  > wrote:
>> 
>> 
>> 
>> Hi Dominic,
>> 
>> 
>> Thanks ! Most of your changes look good.
>> 
>> 
>> A few places where there are questions are:
>> 
>> 
>> configure.ac <http://configure.ac> -------------------- Referring
>> to line : "CFLAGS="-pedantic -Wall -Werror -Wno-unused-result
>> -fno-strict-aliasing -std=gnu99" 1. The -Wno-unused-result option
>> is not available with CentOS 5. So, this cannot be included in
>> NDT right now.
>> 
>> 
>> Ok. That's only there due to -Werror, and because it's not
>> always that useful a warning.
>> 
>> 
>> 
>> 
>> 2. It may be better to skip the -Werror for now. We may be able
>> to include it in later versions after we have had a chance to
>> test it with more environments. Were you just including it to
>> help with removing warnings? Also, with a few other libraries
>> (that may not have been installed on your test/build system), I
>> saw "errors" which were actually warnings, but being interpreted
>> as "errors" with this option.
> 
> 
> To confirm - I will remove -Werror and -Wno-unused-result before 
> committing the patch.
> 
> 
>> 
>> 
>> Exactly - it helped me track down all the warnings and on my
>> local build I was warning (error) free with this combination.
>> Which felt really nice. Not including it is fine, but it does
>> mean that warnings that are actually important (pointer/int casts
>> of different lengths, wrong types being passed to functions) can
>> be ignored.
>> 
>> 
>> 
>> 
>> 3. We may also want to skip the -fno-strict aliasing for now.
>> 
>> 
>> Sadly it's impossible to do any work with the socket interface
>> and have strict aliasing enabled. The API relies on casts between
>> types that violate strict aliasing so warning about it is not
>> useful in the slightest.
>> 
> Ok, lets include this option.
>> 
> 
>> logging.c -------------- Referring to changes in char
>> *get_currenttime(char *isoTime, int isotimearrsize) : Can you
>> please explain your intention in doing this? We are tending 
>> towards not having this change for now.
>> 
>> 
>> The code that was there is wrong: gmtime takes a time_t*, not a 
>> timeval. Also in the current code, timenow and tv are unused.
> Thanks, lets include this.
>> 
>> 
>> 
>> 
>> 
>> I built/installed the server+client with just the "-std=99"
>> CFLAG option, and the results looked comparable to the previous
>> version. (Some files you changed are included in the server side
>> too).
>> 
>> 
>> That's expected - much of my changes fix warnings that were
>> present in c89 too. The only difference is that c89 fills the
>> warning list with comment format issues which are just noise.
>> 
>> 
>> 
>> 
>> I did not concentrate on IPv6 option testing (your changes in
>> troute6.c), but recall MLAB being involved in some work relating
>> to that. It would be great if you verified that section.
>> 
>> 
>> I've tested the client against the server that is running on
>> m-lab currently, however that is version 3.4, iirc. One of the
>> things I would like to do is get the m-lab servers running a
>> newer version of NDT but that requires us to merge our branch
>> with trunk (one way or the other).
> Are you referring to this patch alone? Aaron will/could have
> already mailed to you about commit access. If its this patch, you
> should (or soon) be able to check in your code to the trunk, unless
> someone else on the ndt-dev team has some review comments.
> 
> 
> I haven't heard anything yet but I'll keep an eye out.
> 
> 
> 
> Or does your branch contain more code you're referring to that 
> you're waiting to check-in? If that is the case, is your branch 
> synchronized with the trunk? If yes, you could initially start
> with building that branch for installation on your M-Lab server. We
> could think about integrating your code in for release 3.6.5.3.
> 
> 
> It's not in this patch - this was me getting warmed up with the 
> code-base and I always find that fixing warnings is a good place to
> start :)
> 
> Our branch is not synchronized with trunk but I'm about to start
> getting it synchronized. Ideally, though, I'd just reimplement the
> few changes we have into trunk directly. I'll put a patch together
> shortly.

To help with that end: Dominic, the revisions of interest for M-Lab,
related to the addition of a client identifier field to the meta
phase/file are 467:644.

If synchronizing the client identifier branch is too problematic, it
may be simpler to re-cut the branch from an up to date trunk and
derive a fresh, good patch from that revision set.


> Also, release 3.6.5.2-rc2 is the "latest" of the trunk, and you
> can get the RPMs(source/otherwise) from
> http://software.internet2.edu to install on your MLab server.
> 
> 
> Thanks
> 
> 
> 
>> 
>> 
>> 
>> 
>> 
>> Thanks, Kavitha
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> On Oct 9, 2012, at 5:44 PM, Dominic Hamon wrote:
>> 
>> 
>> 
>> Hi Kavitha
>> 
>> As requested, I have attached the patch. This fixes a few subtle 
>> issues including out-of-bounds array accesses and invalid
>> pointer/int conversion and occasions where the wrong type was
>> being passed
>> 
>> 
>> 
>> 
>> 
>> methods.
>> 
>> I've tested it by pointing the client at a running known-good
>> server.
>> 
>> Thanks - dominic
>> 
>> On Mon, Oct 08, 2012 at 04:46:10PM -0400, Kavitha Kumar wrote:
>> 
>> 
>> Hi Dominic,
>> 
>> 
>> 
>> 
>> 
>> Pls send the patches to this list for review. Folks here on
>> ndt-dev can review and/or apply them. We can then decide further
>> steps to committing the changes.
>> 
>> 
>> 
>> 
>> 
>> Do you foresee contributing to NDT in a bigger fashion soon/in
>> future and thus like commit access?
>> 
>> 
>> 
>> 
>> 
>> thanks,
>> 
>> 
>> Kavitha
>> 
>> 
>> 
>> 
>> 
>> ----- Original Message -----
>> 
>> 
>> 
>> 
>> From: 
>> 
>> <mailto:>
>> 
>> 
>> 
>> 
>> To: 
>> 
>>  
>> <mailto:>
>> 
>> 
>> 
>> 
>> Sent: Monday, October 8, 2012 3:29:16 PM
>> 
>> 
>> 
>> 
>> Subject: [ndt-dev] Submitting a patch
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> Hello
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> I have prepared a patch that fixes a couple of issues with
>> trunk.
>> 
>> 
>> 
>> 
>> Mostly
>> 
>> 
>> 
>> 
>> innocuous changes to clean up C90 warnings but some of the
>> changes
>> 
>> 
>> 
>> 
>> fix things
>> 
>> 
>> 
>> 
>> like implicit int/pointer conversion.
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> How do I go about submitting the patch for review?
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> Thanks
>> 
>> 
>> 
>> 
>> - dominic
>> 
>> 
>> 
>> 
>> 
>> <fix-warnings.patch>
>> 
>> 
> 
> 


- -- 
Thomas Gideon
Technical Director, Open Technology Institute
New American Foundation
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBCgAGBQJQfW6lAAoJEMkpuTb4IXyoKhUQAJm3qkKX+Bu2kC/BO8BAZsQ9
TSkdlgvvPzWOb6jpBZpEnuwcr/Su3Scg36VeZ9yoRZWXUt39LozKe46LSHuwlHkl
cHCWSg1cotD/EpI8taxikoDCgV6kaeTJFu6yE2I7ugbB5Vjo6RgHihimQhZzHn8w
fJOiCFgCjfznRDpS1BV7QiZ4Yade8KeTTgpp+ou6SLNX63xho91JZkv/nh7WxuUQ
H1G2QMfD49yNZD1jNSQ57sfPcIcdrG2gYEafbSTEV8W0ktD7X0GnSUj3WY8ejPNJ
hNT9zWlyixNCdEeMplT8HxPCu/U5i7kKL53o0FgI9NHN9cvKIUpA8dnCXb+6oYcp
IUuTahymjWd1XCNOVrnzYzDhAKHOw/4h/Zw821By0eTcyy4lVKsEl7yPM31oA64x
bafB0oIBRGgWE0pmQgtLmnbXX5EHdKODA+hPLkzAkwqX/sfijomqMfLxL8E/YVng
egmtKSaBopJZvnCkmJgxFyo1RojSk46RtEO+TJM4pUkJ6EbuNo+0V7BUkOe23Wry
bPln1s+4W+F/iH3dVHwcRw8wdp/fCHvR79uTYdq1Bll3BuSKpW7D1aATV3F60uax
T5QFHz+mXgXKdbkRme2jO+4bwPgSDj7HllDRnCbYNmRbhKRYABsa/rbjyPdmiGix
in1uoN28zCmei71bYs+T
=np8s
-----END PGP SIGNATURE-----