[2/2] alfred: Adding time_subtract utility function

Message ID 1472157596-19774-2-git-send-email-jhaws@sdl.usu.edu
State Superseded, archived
Delegated to: Simon Wunderlich
Headers

Commit Message

Jonathan Haws Aug. 25, 2016, 8:39 p.m. UTC
  Signed-off-by: Jonathan Haws <jhaws@sdl.usu.edu>

Added time_subtract() utility function to use for simply subtracting
one timespec from another.
---
 alfred.h |  2 ++
 server.c |  5 ++++-
 util.c   | 13 +++++++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)
  

Comments

Sven Eckelmann Aug. 26, 2016, 6:45 a.m. UTC | #1
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
  
Jonathan Haws Aug. 26, 2016, 3:02 p.m. UTC | #2
On Fri, 2016-08-26 at 08:45 +0200, Sven Eckelmann wrote:
> 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>

> 

> 

> [...]


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?


> > 

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

> > 

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

> > 

> > +

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


> 

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

Thanks!
  
Sven Eckelmann Aug. 28, 2016, 7:17 a.m. UTC | #3
On Freitag, 26. August 2016 15:02:51 CEST Jonathan Haws wrote:
[...]
> 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?

Now the "Signed-off-by: Jonathan Haws <Jonathan.Haws@sdl.usu.edu>" is missing
at the end. You should check out "11) Sign your work" [2] to understand why
Linux related submissions tend to have this line.

The reason why it must be at the end was shown in my last mail. Tools like
git-am or git-format-patch can automatically add the Signed-off-by of the
person who accepts/forwards the patch - but only does it at the end of the
commit message. So you Signed-off-by should also be at the end of the commit
message.

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

My reading was that Simon wanted just a comment which explains what is does 
(because it was not intuitive to him).

@Simon: can you clarify it for both of us? See also the following question 
from Jonathan:

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

(see above)

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

We prefer not to have a wasted return [4]. But lets wait for Simon's answer
regarding the extra function.

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

Looks like following line in the manpage has an extra space at the end:

    +ALFRED_INTERVAL setting of 10 seconds will be used. Fractional seconds are 

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

batman-adv is a a kernel module (included in the official Linux kernel) and
thus we have to follow the linux kernel coding style [1] and submission 
guidelines [2]. The batctl and alfred patches use the same style and we want to 
have patches have submitted the same way [3]. But we don't tend to enforce 
everything for alfred/batctl because these patches don't have to be forwarded 
to the next "higher" maintainer. But odd things will still be pointed out. And
they will also sometimes result in rejections but this depends on the
maintainer and level of "oddity", 

Kind regards,
	Sven


[1] https://www.kernel.org/doc/Documentation/CodingStyle
[2] https://www.kernel.org/doc/Documentation/SubmittingPatches
[3] https://www.open-mesh.org/projects/open-mesh/wiki/Contribute#Submitting-patches
[4] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl
    search for RETURN_VOID
  
Simon Wunderlich Sept. 6, 2016, 7:39 a.m. UTC | #4
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-patches

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
  

Patch

diff --git a/alfred.h b/alfred.h
index 5b7e965..194c62a 100644
--- a/alfred.h
+++ b/alfred.h
@@ -196,6 +196,8 @@  int netsock_own_address(const struct globals *globals,
 /* util.c */
 int time_diff(struct timespec *tv1, struct timespec *tv2,
 	      struct timespec *tvdiff);
+void time_subtract(struct timespec *tv1, struct timespec *tv2,
+	      struct timespec *tvout);
 void time_random_seed(void);
 uint16_t get_random_id(void);
 bool is_valid_ether_addr(uint8_t *addr);
diff --git a/server.c b/server.c
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);
+
 		if (!time_diff(&last_check, &now, &tv)) {
 			tv.tv_sec = 0;
 			tv.tv_nsec = 0;
diff --git a/util.c b/util.c
index c7e11cc..1b67f78 100644
--- a/util.c
+++ b/util.c
@@ -41,6 +41,19 @@  int time_diff(struct timespec *tv1, struct timespec *tv2,
 	return (tvdiff->tv_sec >= 0);
 }
 
+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;
+	}
+
+	return;
+}
+
 void time_random_seed(void)
 {
 	struct timespec now;