<div dir="ltr">I see. Thank you.<br><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 16, 2016 at 5:04 AM David Gibson <<a href="mailto:david@gibson.dropbear.id.au">david@gibson.dropbear.id.au</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Feb 15, 2016 at 06:29:10PM +0000, Dan Good wrote:<br>
> I thought I understood from our emails at the end of last year that a<br>
> single "polished" commit was desirable. Is that not the same thing as<br>
> taking several very small commits and bundling them into one?<br>
<br>
Often not, in practice. The ultimate goal is reviewability (both<br>
before merging and when looking back at the commit history). But<br>
that's often accomplished different ways depending on the situation.<br>
In particular it tends to be different for initial commits of new code<br>
from changes to existing code.<br>
<br>
For initial commits, it's generally best to elide patches which<br>
correct false steps made during development, or really anything which<br>
doesn't increase the total code size or complexity significantly.<br>
That gives less commits to review, and cleaner code in them to<br>
understand.<br>
<br>
For changes to existing code, however, the important thing is<br>
understanding why the change is desirable and then that the changes<br>
actually accomplish that goal. That tends towards small, single<br>
purpose commits, so it's easy to see what changes belong to what goal<br>
(explained in the commit message).<br>
<br>
Of course, that's a very broad rule of tumb, with plenty of exceptions<br>
and wiggle room.<br>
<br>
> On Mon, Feb 15, 2016 at 5:53 AM David Gibson <<a href="mailto:david@gibson.dropbear.id.au" target="_blank">david@gibson.dropbear.id.au</a>><br>
> wrote:<br>
><br>
> > On Wed, Feb 03, 2016 at 11:42:11PM +0000, Dan Good wrote:<br>
> > > "messy bitser" - what does that mean? That's bad, isn't it?<br>
> ><br>
> > It's... mildly bad.<br>
> ><br>
> > By "bitser" I mean that it contains a number of changes that aren't<br>
> > closely related to each other. In general that makes a patch more<br>
> > difficult to review. How important that is depends on the complexity<br>
> > of the changes and how mature the code being altered is, so it's not<br>
> > really a big deal.<br>
> ><br>
> > The fact that altstack has been breaking Travis builds for a while<br>
> > probably predisposed me to be uncharitable.<br>
> ><br>
> > > Can I help fix the builds?<br>
> ><br>
> > So, I've figured out what the problem is, patch coming shortly.<br>
> ><br>
> > ><br>
> > > On Wed, Feb 3, 2016 at 4:28 PM David Gibson <<a href="mailto:david@gibson.dropbear.id.au" target="_blank">david@gibson.dropbear.id.au</a><br>
> > ><br>
> > > wrote:<br>
> > ><br>
> > > > On Mon, Feb 01, 2016 at 02:43:16AM +0000, Dan Good wrote:<br>
> > > > ><br>
> > > > > * add altstack_remn, returns amount of stack remaining<br>
> > > > > * increase mapping by 1 page to handle abutment case<br>
> > > > > * capture rsp earlier<br>
> > > > > * align stack to 16 bytes<br>
> > > > ><br>
> > > > > Signed-off-by: Dan Good <<a href="mailto:dan@dancancode.com" target="_blank">dan@dancancode.com</a>><br>
> > > ><br>
> > > > Bit of a messy bitser, but doesn't look it will make anything<br>
> > > > worse. so go ahead and push.<br>
> > > ><br>
> > > > Unfortunately, altstack is breaking travis builds at the moment (see,<br>
> > > > e.g. <a href="https://travis-ci.org/dgibson/ccan/jobs/106661592" rel="noreferrer" target="_blank">https://travis-ci.org/dgibson/ccan/jobs/106661592</a>) - but not<br>
> > > > local builds for me which is making it difficult to debug.<br>
> > > ><br>
> > > > Some investigation showed that the test prog was getting a SIGBUS, but<br>
> > > > I didn't get further than that.<br>
> > > ><br>
> > > > altstack also breaks "make all" on any non-x86_64 platform (including<br>
> > > > 32-bit x86) which is pretty horrid. For that I think we need to<br>
> > > > adjust the makefiles to look at rusty's new "ported" stuff from _info.<br>
> > > ><br>
> > > ><br>
> > > > > ---<br>
> > > > > ccan/altstack/altstack.c | 17 ++++++++++++++---<br>
> > > > > ccan/altstack/altstack.h | 14 ++++++++++++++<br>
> > > > > ccan/altstack/test/run.c | 20 +++++++++++++-------<br>
> > > > > 3 files changed, 41 insertions(+), 10 deletions(-)<br>
> > > > ><br>
> > > > > diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c<br>
> > > > > index 6faf38f..b71c64f 100644<br>
> > > > > --- a/ccan/altstack/altstack.c<br>
> > > > > +++ b/ccan/altstack/altstack.c<br>
> > > > > @@ -8,6 +8,7 @@<br>
> > > > > #include <signal.h><br>
> > > > > #include <stdio.h><br>
> > > > > #include <string.h><br>
> > > > > +#include <unistd.h><br>
> > > > > #include <sys/mman.h><br>
> > > > ><br>
> > > > > static __thread char ebuf[ALTSTACK_ERR_MAXLEN];<br>
> > > > > @@ -37,6 +38,11 @@ static void segvjmp(int signum)<br>
> > > > > }<br>
> > > > ><br>
> > > > > static __thread void *rsp_save_[2];<br>
> > > > > +static __thread rlim_t max_;<br>
> > > > > +<br>
> > > > > +rlim_t altstack_max(void) {<br>
> > > > > + return max_;<br>
> > > > > +}<br>
> > > > ><br>
> > > > > static ptrdiff_t rsp_save(unsigned i) {<br>
> > > > > assert(i < 2);<br>
> > > > > @@ -57,6 +63,7 @@ static __thread void *arg_, *out_;<br>
> > > > ><br>
> > > > > int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)<br>
> > > > > {<br>
> > > > > + long pgsz = sysconf(_SC_PAGESIZE);<br>
> > > > > int ret = -1, undo = 0;<br>
> > > > > char *m;<br>
> > > > > struct rlimit rl_save;<br>
> > > > > @@ -69,11 +76,16 @@ int altstack(rlim_t max, void *(*fn)(void *),<br>
> > void<br>
> > > > *arg, void **out)<br>
> > > > > fn_ = fn;<br>
> > > > > arg_ = arg;<br>
> > > > > out_ = 0;<br>
> > > > > + max_ = max;<br>
> > > > > ebuf[elen = 0] = '\0';<br>
> > > > > if (out) *out = 0;<br>
> > > > ><br>
> > > > > + // if the first page below the mapping is in use, we get<br>
> > max-pgsz<br>
> > > > usable bytes<br>
> > > > > + // add pgsz to max to guarantee at least max usable bytes<br>
> > > > > + max += pgsz;<br>
> > > > > +<br>
> > > > > ok(getrlimit(RLIMIT_STACK, &rl_save), 1);<br>
> > > > > - ok(setrlimit(RLIMIT_STACK, &(struct rlimit) { max,<br>
> > > > rl_save.rlim_max }), 1);<br>
> > > > > + ok(setrlimit(RLIMIT_STACK, &(struct rlimit) { max_,<br>
> > > > rl_save.rlim_max }), 1);<br>
> > > > > undo++;<br>
> > > > ><br>
> > > > > ok(m = mmap(0, max, PROT_READ|PROT_WRITE,<br>
> > > > MAP_PRIVATE|MAP_ANONYMOUS|MAP_GROWSDOWN|MAP_NORESERVE, -1, 0), 1);<br>
> > > > > @@ -91,8 +103,7 @@ int altstack(rlim_t max, void *(*fn)(void *), void<br>
> > > > *arg, void **out)<br>
> > > > > ok(sigaction(SIGSEGV, &sa, &sa_save), 1);<br>
> > > > > undo++;<br>
> > > > ><br>
> > > > > - asm volatile ("movq %%rsp, %%r10\nmov %0, %%rsp\npush<br>
> > > > %%r10" : : "g" (m + max) : "r10");<br>
> > > > > - rsp_save(0);<br>
> > > > > + asm volatile ("movq %%rsp, %%r10\nmov %1, %%rsp\nmov<br>
> > > > %%rsp, %0\nsub $8, %%rsp\npush %%r10" : "=g" (rsp_save_[0]) : "g" (m +<br>
> > max)<br>
> > > > : "r10");<br>
> > > > > out_ = fn_(arg_);<br>
> > > > > asm volatile ("pop %rsp");<br>
> > > > > ret = 0;<br>
> > > > > diff --git a/ccan/altstack/altstack.h b/ccan/altstack/altstack.h<br>
> > > > > index 5570e7b..4445a2a 100644<br>
> > > > > --- a/ccan/altstack/altstack.h<br>
> > > > > +++ b/ccan/altstack/altstack.h<br>
> > > > > @@ -104,6 +104,20 @@ char *altstack_geterr(void);<br>
> > > > > ptrdiff_t altstack_used(void);<br>
> > > > ><br>
> > > > > /**<br>
> > > > > + * altstack_max - return usable stack size<br>
> > > > > + *<br>
> > > > > + * Returns: max value from altstack() call<br>
> > > > > + */<br>
> > > > > +rlim_t altstack_max(void);<br>
> > > > > +<br>
> > > > > +/**<br>
> > > > > + * altstack_remn - return amount of stack remaining<br>
> > > > > + *<br>
> > > > > + * Returns: altstack_max() minus altstack_used()<br>
> > > > > + */<br>
> > > > > +#define altstack_remn() (altstack_max() - altstack_used())<br>
> > > > > +<br>
> > > > > +/**<br>
> > > > > * altstack_rsp_save - set initial rsp value<br>
> > > > > *<br>
> > > > > * Capture the current value of rsp for future altstack_used()<br>
> > > > > diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c<br>
> > > > > index adc1020..d0b8d28 100644<br>
> > > > > --- a/ccan/altstack/test/run.c<br>
> > > > > +++ b/ccan/altstack/test/run.c<br>
> > > > > @@ -4,6 +4,7 @@<br>
> > > > > #include <setjmp.h><br>
> > > > > #include <signal.h><br>
> > > > > #include <string.h><br>
> > > > > +#include <unistd.h><br>
> > > > > #include <sys/mman.h><br>
> > > > > #include <ccan/tap/tap.h><br>
> > > > > #include <ccan/altstack/altstack.h><br>
> > > > > @@ -20,13 +21,13 @@ enum {<br>
> > > > > };<br>
> > > > > int fail, call1, call2;<br>
> > > > > char *m_;<br>
> > > > > -rlim_t max_;<br>
> > > > > +rlim_t msz_;<br>
> > > > > #define e(x) (900+(x))<br>
> > > > > #define seterr(x) (errno = e(x))<br>
> > > > > #define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno ||<br>
> > > > out_ ? (x) : 0))<br>
> > > > > #define getrlimit(...) (fail&getrlimit_ ?<br>
> > > > (seterr(getrlimit_), -1) : (setcall(getrlimit_),<br>
> > > > getrlimit(__VA_ARGS__)))<br>
> > > > > #define mmap(...) (fail&mmap_ ?<br>
> > (seterr(mmap_),<br>
> > > > (void *)-1) : (setcall(mmap_), mmap(__VA_ARGS__)))<br>
> > > > > -#define munmap(a, b) (fail&munmap_ ?<br>
> > > > (seterr(munmap_), -1) : (setcall(munmap_),<br>
> > > > munmap(m_=(a), max_=(b))))<br>
> > > > > +#define munmap(a, b) (fail&munmap_ ?<br>
> > > > (seterr(munmap_), -1) : (setcall(munmap_),<br>
> > > > munmap(m_=(a), msz_=(b))))<br>
> > > > > #define setrlimit(...) (fail&setrlimit_ ?<br>
> > > > (seterr(setrlimit_), -1) : (setcall(setrlimit_),<br>
> > > > setrlimit(__VA_ARGS__)))<br>
> > > > > #define sigaltstack(...) (fail&sigaltstack_ ?<br>
> > > > (seterr(sigaltstack_), -1) : (setcall(sigaltstack_),<br>
> > > > sigaltstack(__VA_ARGS__)))<br>
> > > > > #define sigaction(...) (fail&sigaction_ ?<br>
> > > > (seterr(sigaction_), -1) : (setcall(sigaction_),<br>
> > > > sigaction(__VA_ARGS__)))<br>
> > > > > @@ -58,7 +59,9 @@ static void *wrap(void *i)<br>
> > > > ><br>
> > > > > int main(void)<br>
> > > > > {<br>
> > > > > - plan_tests(16);<br>
> > > > > + long pgsz = sysconf(_SC_PAGESIZE);<br>
> > > > > +<br>
> > > > > + plan_tests(17);<br>
> > > > ><br>
> > > > > #define chkfail(x, y, z, c1, c2) (call1 = 0, call2 = 0, errno = 0,<br>
> > > > ok1((fail = x) && (y) && errno == (z) && call1 == (c1) && call2 ==<br>
> > (c2)));<br>
> > > > > #define chkok( y, z, c1, c2) (call1 = 0, call2 = 0, errno = 0,<br>
> > fail<br>
> > > > = 0, ok1((y) && errno == (z) && call1 == (c1) && call2 == (c2)));<br>
> > > > > @@ -86,7 +89,7 @@ int main(void)<br>
> > > > > chkfail(munmap_, altstack(8*MiB, wrap, 0, 0) == 1,<br>
> > > > e(munmap_),<br>
> > > > > getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,<br>
> > > > > setrlimit_|sigaltstack_|sigaction_);<br>
> > > > > - if (fail = 0, munmap(m_, max_) == -1)<br>
> > > > > + if (fail = 0, munmap(m_, msz_) == -1)<br>
> > > > > err(1, "munmap");<br>
> > > > ><br>
> > > > > chkok( altstack(1*MiB, wrap, (void *)<br>
> > 1000000, 0)<br>
> > > > == -1, EOVERFLOW,<br>
> > > > > @@ -102,10 +105,12 @@ int main(void)<br>
> > > > > chkfail(munmap_, altstack(1*MiB, wrap, (void *)<br>
> > 1000000, 0)<br>
> > > > == -1, EOVERFLOW,<br>
> > > > > getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,<br>
> > > > > setrlimit_|sigaltstack_|sigaction_);<br>
> > > > > - if (fail = 0, munmap(m_, max_) == -1)<br>
> > > > > + if (fail = 0, munmap(m_, msz_) == -1)<br>
> > > > > err(1, "munmap");<br>
> > > > ><br>
> > > > > - ok1(used > 1*MiB-1*KiB && used < 1*MiB);<br>
> > > > > + ok1(altstack_max() == 1*MiB);<br>
> > > > > + diag("used: %lu", used);<br>
> > > > > + ok1(used >= 1*MiB && used <= 1*MiB + pgsz);<br>
> > > > ><br>
> > > > > char *p;<br>
> > > > > for(p = altstack_geterr(); *p; p++)<br>
> > > > > @@ -128,7 +133,8 @@ int main(void)<br>
> > > > > getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,<br>
> > > > > setrlimit_|munmap_|sigaltstack_|sigaction_);<br>
> > > > ><br>
> > > > > - ok1(used > 8*MiB-8*KiB && used < 8*MiB);<br>
> > > > > + diag("used: %lu", used);<br>
> > > > > + ok1(used >= 8*MiB && used <= 8*MiB + pgsz);<br>
> > > > ><br>
> > > > > used = 0;<br>
> > > > > chkok( altstack(8*MiB, wrap, (void *) 100000,<br>
> > 0)<br>
> > > > == 0, 0,<br>
> > > ><br>
> > > ><br>
> ><br>
> ><br>
<br>
--<br>
David Gibson | I'll have my music baroque, and my code<br>
david AT <a href="http://gibson.dropbear.id.au" rel="noreferrer" target="_blank">gibson.dropbear.id.au</a> | minimalist, thank you. NOT _the_ _other_<br>
| _way_ _around_!<br>
<a href="http://www.ozlabs.org/~dgibson" rel="noreferrer" target="_blank">http://www.ozlabs.org/~dgibson</a><br>
</blockquote></div></div>