NDT-DEV email list created

[Dominic Hamon <>]

On Mon, Oct 15, 2012 at 1:25 PM, Kavitha Kumar <> wrote:

Hi Dominic,

  Sorry about the delayed reply. My comments inline!

Thanks,
Kavitha

----- Original Message -----

> From: "Dominic Hamon" <>
> To: "Kavitha Kumar" <>
> Cc:
> 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 <
> > 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.

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.

 

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