On 08/09/15 01:28, Richard PALO wrote:
Le 07/09/15 13:50, David Sainty a écrit :Hi Richard,http://www.netbsd.org/~richard/pdksh.patch The above patch is the pkgsrc specific WIP of a tiny bit of discussion in tech-userlevel@ I'd like to sollicit any comments/observations prior to committing. I've been using this for a bit now.--- files/table.h +++ files/table.h @@ -6,7 +6,7 @@struct table {Area *areap; /* area to allocate entries */ - short size, nfree; /* hash size (always 2^^n), free entries */ + int size, nfree; /* hash size (always 2^^n), free entries */ struct tbl **tbls; /* hashed table items */ }; Indeed! This bit of your patch is absolutely identical to mine. So yes, with this patch in place you won't see the crash either. Good work :) Shame I hadn't noticed you'd done it first... --- files/table.c +++ files/table.c @@ -8,6 +8,10 @@#define INIT_TBLS 8 /* initial table size (power of 2) */ +#ifndef min+#define min(a, b) ((a) > (b) ? (b) : (a)) +#endif + static void texpand ARGS((struct table *tp, int nsize)); static int tnamecmp ARGS((void *p1, void *p2));@@ -116,7 +120,7 @@}if (tp->nfree <= 0) { /* too full */- texpand(tp, 2*tp->size); + texpand(tp, 2 * min(INT_MAX/2, tp->size)); goto Search; } This part I have some problems with, I don't think it should go in.the discussion was, as mentioned in tech-pkgs, actually started on tech-userlevel@ on 17/05. the other patches where as requested to bring pkgsrc up to native parity.
Heh, I'd forgotten there even was a ksh in the base :) It doesn't seem so important when we have such an excellent /bin/sh. So, I read through the userlevel discussion...
I'm still lost on the heritage of all the bits of your patch though. On the face of it I don't think anyone will complain about merging improvements from NetBSD's base into Pkgsrc pdksh.
I think changing the short to an int in base is sensible. With relation to the INT_MAX limit (for which I definitely don't have hardware capable of testing!), I don't want the min(INT_MAX) change because it gives the impression that it's a problem that has been solved. The "correct" complete fix is to change those short/int fields into size_t fields (because they actually are representations of the size of objects in memory), but that is a change that needs many more lines of patching (more type changes), and more vetting of consequences (does anything break if the size fields become unsigned?).
I'm mostly concerned with testability of the rest. And the size of the patch :) I'd rather see it broken down into separate phases where it's clear to observers what's happening: Probably change the short to an int in NetBSD base because it's small and testable; Bring Pkgsrc up to date with NetBSD base; perhaps change the short/int to a size_t; and perhaps then decide whether to pull in that "rv" patch from upstream.
I'm pretty dubious about the "rv" patch though. It's quite difficult to tell exactly what the patch's actual functional difference is, and what specific problem it solves; and if there is no test case for the problem it's fixing, no-one has described the functional change, and it hasn't been pulled into the standard distribution for 8 years, maybe it should be left alone until one of those things happens...