NDT-DEV email list created

[Dominic Hamon <>]

On Thu, Oct 11, 2012 at 11:35 AM, Kavitha Kumar <> wrote:

Hi Dominic,

      Thanks ! Most of your changes look good. 

A few places where there are questions are:

 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.

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.

 

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.

 

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

 

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:

To:

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>