Re: [B.A.T.M.A.N.] [PATCH 2/2] alfred: Adding time_subtract utility function

Hi Jonathan,

On Friday, August 26, 2016 3:02:51 PM CEST Jonathan Haws wrote:

On Fri, 2016-08-26 at 08:45 +0200, Sven Eckelmann wrote:

...

You don't seem to follow these rules yet. Instead you seem switch the Signed- off-by: line with the body of the explanation. So a git-am with -s (as the maintainers do here) would then produce this mess:

alfred: Adding time_subtract utility function

Signed-off-by: Jonathan Haws <jhaws@sdl.usu.edu>

Added time_subtract() utility function to use for simply

subtracting one timespec from another. Signed-off-by: Sven Eckelmann sven@narfation.org

[...]

I've backed up to origin/master and will produce a single patch. The "signed off by:" was a misunderstanding on my part. Here is what the commit log will look like:

alfred: Externalized synchronization interval

ALFRED_INTERVAL is now externalized via the -p option (synchronization period). If specified as option, user supplied interval is used, otherwise the default interval of ALFRED_INTERVAL is used.

Does that look right?

Yes - and you put your Signed-off after this text and you are good. :)

...

...

index c5df945..884a1a7 100644 --- a/server.c +++ b/server.c @@ -372,7 +372,10 @@ int alfred_server(struct globals *globals)

while (1) { clock_gettime(CLOCK_MONOTONIC, &now);

• time_diff(&now, &globals->sync_period, &now);
  

• /* subtract the synchronization period from the
  

current time */

• time_subtract(&now, &globals->sync_period, &now);
  

I'm guessing Simon meant to have the comment in the first patch. I don't get why you now introduce a time_subtract which is just a copy of time_diff (without the return value).

My reading of his response was that time_diff was meant for something completely different, thus shouldn't be used here. Thus I created time_subtract.

Sorry, I guess I wasn't clear here. Yes, time_diff was for an entirely different purpose, but re-creating the function with a different name is not a better solution IMHO.

...

...

+void time_subtract(struct timespec *tv1, struct timespec *tv2,

•      struct timespec *tvout) {
  

• tvout->tv_sec = tv1->tv_sec - tv2->tv_sec;
• if (tv1->tv_nsec < tv2->tv_nsec) {

• tvout->tv_nsec = 1000000000 + tv1->tv_nsec - tv2-
  
  

...

tv_nsec;

• tvout->tv_sec -= 1;
  

• } else {

• tvout->tv_nsec = tv1->tv_nsec - tv2->tv_nsec;
  

• }

Have you checked the glibc documentation about this topic (search for timeval_subtract)?

https://www.gnu.org/software/libc/manual/html_node/Elapsed-Time.html

This might be relevant when you want to create an function which does handle more things than time_diff currently does. Otherwise I don't see a reason why we just have the same code twice in util.c

Yeah, I've looked at that. The code they present there essentially does the same thing. I can pull that in and have a timespec_subtract call, or I can go back to time_diff with the comment. Which would you prefer?

Both are fine. Either do time_diff with a comment in alfred_server() to let the reader know that you are somewhat mistreating the function, or pull the timespec_subtract() function in (I assume we can do that, license wise). You decide. ;)

...

...

• return;

+}

Why a return here?

Style. I never have a function without a return, even voids. Maybe I'm a bit OCD on that point, but I just feel happier seeing it there. :) Depending on what you want to do with time_subtract, we can remove this return or replace the function.

I would prefer not to have that useless return here. :)

...

Btw. I am seeing whitespace warnings when applying the patches. Maybe you can check them:

Applying patch #16667 using 'git am' Description: [1/2] alfred: Externalized synchronization interval Applying: alfred: Externalized synchronization interval .git/rebase-apply/patch:89: trailing whitespace. ALFRED_INTERVAL setting of 10 seconds will be used. Fractional seconds are warning: 1 line adds whitespace errors. Applying patch #16668 using 'git am' Description: [2/2] alfred: Adding time_subtract utility function Applying: alfred: Adding time_subtract utility function

I'm not sure where this would have cropped up, but I'll double check it before I submit the next patch version.

I'm new to the contributing game - I've read through the wiki, but apparently there are other details that I missed. Is there some good documentation on the typical procedure to follow and formats and whatnot to read up on? I thought I had understood everything, but apparently not. I hate to cause extra work for you maintainers simply because I can't get a simple patch right.

Don't worry, as long as you are patient with us we are patient with you. ;) And I admit we have quite a list of rules. Basically, we are following the Linux kernel style, and there are a couple of tools which can help checking that (e.g. checkpatch.pl). The good news is, once you do it like we do, you also know how to format patches for the Linux kernel (more or less). ;)

Did you read the following wiki page yet?

https://www.open-mesh.org/projects/open-mesh/wiki/Contribute#Submitting-patc...

White space warnings can probably be avoided if you use git format-patch to create your patch, and git send-email to send it to the mailing list. This avoids any mangling of the patch which git wouldn't like.

Thanks for bearing with us. :) Simon