Re: trivial fix for pmemrange
On 17/02/14 10:11, Kieran Devlin wrote: the original implementation use identical logic for both ‘high’ & ‘low’, which will cause ‘high’ & ‘low’ end up at same RB-tree node, instead of an expected interval. and finally, break ‘for’ loop logic luckily all these conditions never meet in practice. Hmm, it may hit when you have a device that requires 2-4GB memory in a 0-4GB memory range, which is not entirely unlikely... (just to give an example). On Feb 17, 2014, at 5:14 PM, Mike Larkin wrote: Probably get a better response if you explained what this diff does and/or fixes… I have to agree with Mike here. As someone who is rather familiar with the code, I still required a fair amount of time to figure out what the original code is doing and how the diff affects it. Remember that not every developer is as familiar with the code as you are. :) i just think this bug is a little too obvious Most bugs are, once you find them. Until that happens, they look perfectly valid. I think the original body of code took something like 30+ revisions... Anyway, to respond to the question that Mike asked: --- Intended behaviour --- The function in question, essentially looks for a page satisfying the request condition: [1] it must intersect with [low_addr - high_addr] [2] it must be of a given memory type It will return a pointer to any (arbitrary) range satisfying these conditions. The implementation is based on lower-bound and upper-bound lookup in a tree. It first clamps the range such that the upper (high) and lower (low) bounds are at or around [1] (i.e. traversing low - high would cover the entirety of [1]). As an optimization, if it finds a range that already satisfies the request, it quickly returns it (this is why the loops contain return statements). As the last step, it traverses the entire range O(n log n) of pages in the range low-high, returning the first match. --- The fix --- Kieran correctly notes that the code does the lower bound lookup is incorrect. Instead of walking downward, it walks upward, essentially performing the same traversal as the loop above (for variable 'high') and ending up at the same position. The final loop therefore traverses an empty range. Instead of using RB_RIGHT, RB_LEFT should be used to select ever lower addresses, up to falling out of the range. --- Additional thoughts --- I doubt that function actually does what it says in the comment, after the fix is applied. I would recommend checking the logic in the last loop as well, since from reading it back, I think it may still exhibit false negatives. I doubt it will trigger false positives though. -- Ariane
signify manpage example
Hi tech, I find the signify manpage regarding the examples a little bit strange. The sign example shows not the relationship between the message that should be signed and the signature file. So I changed the manpage a bit to be more clear. Regards, Fritjof Index: signify.1 === RCS file: /cvs/src/usr.bin/signify/signify.1,v retrieving revision 1.22 diff -u -p -u -r1.22 signify.1 --- signify.1 17 Jan 2014 03:38:12 - 1.22 +++ signify.1 27 Feb 2014 13:58:05 - @@ -126,10 +126,10 @@ Create a new keypair: .Dl $ signify -G -p newkey.pub -s newkey.sec .Pp Sign a file, specifying a signature name: -.Dl $ signify -S -s key.sec -m message.txt -x msg.sig +.Dl $ signify -S -s key.sec -m message.txt -x message.txt.sig .Pp Verify a signature, using the default signature name: -.Dl $ signify -V -p key.pub -m generalsorders.txt +.Dl $ signify -V -p key.pub -m message.txt .Pp Verify a release directory containing .Pa SHA256.sig
removed unneeded variable in signify
Hi tech, this diff removes an unneeded variable from signify. The int is only used for a comparison in order to make sure the b64 encoding was right. Regards, Fritjof Index: signify.c === RCS file: /cvs/src/usr.bin/signify/signify.c,v retrieving revision 1.41 diff -u -p -u -r1.41 signify.c --- signify.c 22 Jan 2014 21:11:03 - 1.41 +++ signify.c 27 Feb 2014 13:17:16 - @@ -136,7 +136,6 @@ static size_t parseb64file(const char *filename, char *b64, void *buf, size_t len, char *comment) { - int rv; char *commentend, *b64end; commentend = strchr(b64, '\n'); @@ -154,8 +153,7 @@ parseb64file(const char *filename, char if (!b64end) errx(1, "missing new line after b64 in %s", filename); *b64end = 0; - rv = b64_pton(commentend + 1, buf, len); - if (rv != len) + if (b64_pton(commentend + 1, buf, len) != len) errx(1, "invalid b64 encoding in %s", filename); if (memcmp(buf, PKALG, 2)) errx(1, "unsupported file %s", filename);