| Message ID | 1472157596-19774-2-git-send-email-jhaws@sdl.usu.edu (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Simon Wunderlich |
| Headers |
Return-Path: <b.a.t.m.a.n-bounces@lists.open-mesh.org> X-Original-To: patchwork@open-mesh.org Delivered-To: patchwork@open-mesh.org Received: from open-mesh.org (localhost [IPv6:::1]) by open-mesh.org (Postfix) with ESMTP id A7D4A82EA5; Thu, 25 Aug 2016 22:40:07 +0200 (CEST) Authentication-Results: open-mesh.org; dmarc=none header.from=sdl.usu.edu Received: from Perses.usurf.usu.edu (unknown [129.123.197.145]) by open-mesh.org (Postfix) with ESMTPS id 0585B82CD8 for <b.a.t.m.a.n@lists.open-mesh.org>; Thu, 25 Aug 2016 22:40:02 +0200 (CEST) Authentication-Results: open-mesh.org; dmarc=none header.from=sdl.usu.edu Received: from EK.usurf.usu.edu (172.31.35.73) by Perses.usurf.usu.edu (172.31.35.68) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Thu, 25 Aug 2016 14:40:00 -0600 Received: from PERSES.usurf.usu.edu (172.31.35.68) by Ek.usurf.usu.edu (172.31.35.73) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Thu, 25 Aug 2016 14:39:59 -0600 Received: from localhost (172.31.22.119) by PERSES.usurf.usu.edu (172.31.35.68) with Microsoft SMTP Server id 15.0.1210.3 via Frontend Transport; Thu, 25 Aug 2016 14:39:59 -0600 From: Jonathan Haws <jhaws@sdl.usu.edu> To: <b.a.t.m.a.n@lists.open-mesh.org> Date: Thu, 25 Aug 2016 14:39:56 -0600 Message-ID: <1472157596-19774-2-git-send-email-jhaws@sdl.usu.edu> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1472157596-19774-1-git-send-email-jhaws@sdl.usu.edu> References: <1472157596-19774-1-git-send-email-jhaws@sdl.usu.edu> MIME-Version: 1.0 Content-Type: text/plain Cc: Jonathan Haws <jhaws@sdl.usu.edu> Subject: [B.A.T.M.A.N.] [PATCH 2/2] alfred: Adding time_subtract utility function X-BeenThere: b.a.t.m.a.n@lists.open-mesh.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n.lists.open-mesh.org> List-Unsubscribe: <https://lists.open-mesh.org/mm/options/b.a.t.m.a.n>, <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=unsubscribe> List-Archive: <http://lists.open-mesh.org/pipermail/b.a.t.m.a.n/> List-Post: <mailto:b.a.t.m.a.n@lists.open-mesh.org> List-Help: <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=help> List-Subscribe: <https://lists.open-mesh.org/mm/listinfo/b.a.t.m.a.n>, <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=subscribe> Reply-To: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n@lists.open-mesh.org> Errors-To: b.a.t.m.a.n-bounces@lists.open-mesh.org Sender: "B.A.T.M.A.N" <b.a.t.m.a.n-bounces@lists.open-mesh.org> |
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
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-patches [4] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl 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-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
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;