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

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 3820 bytes --]

On Donnerstag, 25. August 2016 14:39:56 CEST Jonathan Haws wrote:
> Signed-off-by: Jonathan Haws <jhaws@sdl.usu.edu>
> 
> Added time_subtract() utility function to use for simply subtracting
> one timespec from another.
> ---

Please add a small changelog when you send a new revision (after the first 
"---"). Please also add the v2/v3/v4/... version to your [PATCH...] prefix for 
patches replacing an older version. These are generated automatically by git-
format-patch when you add the parameter "-v 2" or "-v 3" or ....

A good example is https://patchwork.open-mesh.org/patch/16663/

Please use the format described in
 https://www.kernel.org/doc/Documentation/SubmittingPatches

    The canonical patch subject line is:
    
    Subject: [PATCH 001/123] subsystem: summary phrase

    The canonical patch message body contains the following:
    
      - A "from" line specifying the patch author (only needed if the person
        sending the patch is not the author).
    
      - An empty line.
    
      - The body of the explanation, line wrapped at 75 columns, which will
        be copied to the permanent changelog to describe this patch.
    
      - The "Signed-off-by:" lines, described above, which will
        also go in the changelog.
    
      - A marker line containing simply "---".
    
      - Any additional comments not suitable for the changelog.
    
      - The actual patch (diff output).

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>


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

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

> +
> +	return;
> +}
> +

Why a 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

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]