Signed-off-by: Jonathan Haws jhaws@sdl.usu.edu
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. --- alfred.h | 1 + main.c | 14 +++++++++++++- man/alfred.8 | 5 +++++ server.c | 4 ++-- 4 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/alfred.h b/alfred.h index a9906c8..5b7e965 100644 --- a/alfred.h +++ b/alfred.h @@ -131,6 +131,7 @@ struct globals { uint16_t changed_data_type_count; /* maximum is 256 */
struct timespec if_check; + struct timespec sync_period;
struct hashtable_t *data_hash; struct hashtable_t *transaction_hash; diff --git a/main.c b/main.c index 9cab705..3aa89fd 100644 --- a/main.c +++ b/main.c @@ -59,6 +59,8 @@ static void alfred_usage(void) printf(" -m, --master start up the daemon in master mode, which\n"); printf(" accepts data from slaves and syncs it with\n"); printf(" other masters\n"); + printf(" -p, --sync-period [period] set synchronization period, in seconds\n"); + printf(" fractional seconds are supported (i.e. 0.2 = 5 Hz)\n"); printf("\n"); printf(" -u, --unix-path [path] path to unix socket used for client-server\n"); printf(" communication (default: ""ALFRED_SOCK_PATH_DEFAULT"")\n"); @@ -156,6 +158,7 @@ out: static struct globals *alfred_init(int argc, char *argv[]) { int opt, opt_ind, i, ret; + double sync_period = 0.0; struct globals *globals; struct option long_options[] = { {"set-data", required_argument, NULL, 's'}, @@ -170,6 +173,7 @@ static struct globals *alfred_init(int argc, char *argv[]) {"update-command", required_argument, NULL, 'c'}, {"version", no_argument, NULL, 'v'}, {"verbose", no_argument, NULL, 'd'}, + {"sync-period", required_argument, NULL, 'p'}, {NULL, 0, NULL, 0}, };
@@ -193,12 +197,14 @@ static struct globals *alfred_init(int argc, char *argv[]) globals->unix_path = ALFRED_SOCK_PATH_DEFAULT; globals->verbose = 0; globals->update_command = NULL; + globals->sync_period.tv_sec = ALFRED_INTERVAL; + globals->sync_period.tv_nsec = 0; INIT_LIST_HEAD(&globals->changed_data_types); globals->changed_data_type_count = 0;
time_random_seed();
- while ((opt = getopt_long(argc, argv, "ms:r:hi:b:vV:M:I:u:dc:", long_options, + while ((opt = getopt_long(argc, argv, "ms:r:hi:b:vV:M:I:u:dc:p:", long_options, &opt_ind)) != -1) { switch (opt) { case 'r': @@ -265,6 +271,12 @@ static struct globals *alfred_init(int argc, char *argv[]) printf("%s %s\n", argv[0], SOURCE_VERSION); printf("A.L.F.R.E.D. - Almighty Lightweight Remote Fact Exchange Daemon\n"); return NULL; + case 'p': + sync_period = strtod(optarg, NULL); + globals->sync_period.tv_sec = (int) sync_period; + globals->sync_period.tv_nsec = (double)(sync_period - (int)sync_period) * 1e9; + printf(" ** Setting sync interval to: %.9f seconds (%ld.%09ld)\n", sync_period, globals->sync_period.tv_sec, globals->sync_period.tv_nsec); + break; case 'h': default: alfred_usage(); diff --git a/man/alfred.8 b/man/alfred.8 index 49d42bc..df37040 100644 --- a/man/alfred.8 +++ b/man/alfred.8 @@ -118,6 +118,11 @@ overhead). \fB-c\fP, \fB--update-command\fP \fIcommand\fP Specify command to execute on data change. It will be called with data-type list as arguments. +.TP +\fB-p\fP, \fB--sync-period\fP \fIperiod\fP +Specify alfred synchronization period, in seconds. If not specified, the default +ALFRED_INTERVAL setting of 10 seconds will be used. Fractional seconds are +supported. . .SH EXAMPLES Start an alfred server listening on bridge br0 (assuming that this bridge diff --git a/server.c b/server.c index 47aee4f..c5df945 100644 --- a/server.c +++ b/server.c @@ -372,7 +372,7 @@ int alfred_server(struct globals *globals)
while (1) { clock_gettime(CLOCK_MONOTONIC, &now); - now.tv_sec -= ALFRED_INTERVAL; + time_diff(&now, &globals->sync_period, &now); if (!time_diff(&last_check, &now, &tv)) { tv.tv_sec = 0; tv.tv_nsec = 0; @@ -409,7 +409,7 @@ int alfred_server(struct globals *globals)
if (globals->opmode == OPMODE_MASTER) { /* we are a master */ - printf("announce master ...\n"); + printf("[%ld.%09ld] announce master ...\n", last_check.tv_sec, last_check.tv_nsec); announce_master(globals); sync_data(globals); } else {
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(-)
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;
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
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!
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-patc... [4] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts... search for RETURN_VOID
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
b.a.t.m.a.n@lists.open-mesh.org