<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Fri, Dec 12, 2014 at 2:37 PM, Carl Worth <<a href="mailto:cworth@cworth.org" target="_blank">cworth@cworth.org</a>> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, Dec 11 2014, Jason Ekstrand wrote: > Yeah, this looks nifty. Thanks! > I've got some comments on the implementation though. I really appreciate the careful review. I obviously didn't pay quite as much attention to detail as I would like to when writing this patch. I'm glad I didn't bury this code in a giant "here's the shader cache" series where this review wouldn't have happened. </blockquote><div> </div><div>No problem. It was a fun little review. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> >> + * Permission is hereby granted, free of charge, to any person obtaining > a [Incidentally, your email to me appears to have been word-wrapped after the quote prefixes were added, (and those prefixes pushing some lines past 80 columns). That made it a bit harder to read. I haven't seen this failure mode commonly before. Part of that may be due to the mesa/.dir-locals.el file which is making my emacs wrap my code at 78 columns rather than its default of 70. Maybe that should be toned down a bit.] </blockquote><div> </div><div>You can probably blame it on G-Mail </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> >> + if (blob->allocated == 0) >> + to_allocate = BLOB_INITIAL_SIZE; >> + else >> + to_allocate = blob->allocated * 2; > > What happens if additional is bigger than blob->size? Maybe iterate (if > you want to keep power-of-two) or take a max here. Good catch. I'm not particularly concerned about a power-of-two size. So I've changed the code to take a max here. >> +static bool >> +align_blob (struct blob *blob, size_t alignment) >> +{ >> + size_t new_size; >> + >> + new_size = ALIGN(blob->size, alignment); > > Do we what to align the pointer or the size? Aligning the size only makes > sense if the pointer is already aligned. Note that the size we're aligning here (blob->size) is the size of all data that's already written to the blob, (not the size of the object about to be written that is imposing an alignment constraint). It's not clear to me if your question was based on a misreading of that. </blockquote><div> </div><div>Right, I think I misread something. We don't really have a pointer to align here. My point was that if the pointer is only 16-aligned then it's no good to 32-align the size because ptr + size will still only be 16-aligned. However, given that it's coming out of malloc and we only care about 64bit things, it should be fine. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > At the very least, we should assert that the requested alignment is > less than or equal to the known pointer alignment for the start of the > blob. The start of the blob's data is a pointer returned by malloc(), (or realloc()), so the known pointer alignment there should be adequate for anything we're writing to it. </blockquote><div> </div><div>Sure. It's probably fine then. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> >> +static void >> +align_blob_reader (struct blob_reader *blob, size_t alignment) >> +{ >> + blob->current = blob->data + ALIGN(blob->current - blob->data, > alignment); > > Again, pointer or size? Also, why is this way up here when the other > reader stuff comes later. I guess I was putting all of the allocation and alignment code in one place, (which wasn't obvious since the blob_reader code doesn't have any allocation). Obviously, that's easy to move. I'll do that. >> +void >> +blob_reader_init (struct blob_reader *blob, uint8_t *data, size_t size) > > Calling it blob is a bit disconcerting, but probably not a big deal. Yes. I hesitated a bit when doing that. This came about probably because I had earlier toyed with having a single, shared data structure for both purposes, but I decided that they really are different. At the very least this "struct blob_reader as 'blob'" naming is contained to just this file and obviously doesn't leak into any callers or anything. >> +void * >> +blob_read_bytes (struct blob_reader *blob, size_t size) >> +{ >> + void *ret; >> + >> + if (blob->current > blob->end || blob->end - blob->current < size) >> + return NULL; > > Why isn't this a call to ensure_can_read? Without this, the overrun flag > never gets set. Thanks for the catch. > We also don't get overrun set for out-of-bounds copies and > string reads. If that's intentional, it should be documented. That was not intentional. Fixing blob_read_bytes() above takes care of out-of-bounds copies. For blob_read_string I've just added: blob->overrun = true; to the case that handles "no 0 byte appears in the remainder of the blob's data". > Also, you reversed the logic wrong. It should be current >= end. I don't think so. Checking for "current >= end" is a check to see whether a previous operation has overrun. But what we want to check here is whether the current operation would overrun, (whether or not the previous one had). But it doesn't matter since I just deleted that code and replaced it with a call to ensure_can_read(). >> +blob_read_string (struct blob_reader *blob) >> +{ >> + int size; >> + char *ret; >> + uint8_t *nul; >> + >> + /* If there is no NULL terminator, the reader gets nothing. */ >> + nul = memchr (blob->current, 0, blob->end - blob->current); > > Should we check for current >= end first?... Yes. That's another good fix, (for the case of trying to read a string immediately after having read the entire blob's data). >> + if (! ensure_can_read(blob, size)) >> + return 0; > > ...If we do, this does nothing. How about for my own comfort, I leave that in as: assert(ensure_can_read(blob, size)); > We may want to set the overrun flag if there is no null. As I mentioned above, I did add this. >> +bool >> +blob_overwrite_bytes (struct blob *blob, >> + size_t offset, >> + const void *bytes, >> + size_t to_write); > > This function isn't implemented. Thanks. As you mentioned in your other email, I moved this chunk to the second patch. I'll follow up with a second revision of this patch incorporating all of the changes described here. -Carl </blockquote></div></div></div>