[Mesa-dev] [PATCH 1/2] glsl: Add blob.c---a simple interface for serializing data

[Carl Worth]

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.

>> + * 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.]

>> +   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.

> 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.

>> +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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141212/43e4df1c/attachment-0001.sig>