Conversation

This series of patches is to support valref treeobjects pointing to zero-length blobref data.

This seems like a lot of code for a pretty dumb corner case, but it came down to updating the KVS cache API to not assume non-NULL data pointer means valid and NULL data pointer is invalid. A flag was placed into the KVS cache entry to handle this, but it beget a number of API style changes.

For starters, cache_entry_get_raw () had to be modified so that a NULL data buffer returned wasn't automatically assumed to be error. This is an obvious refactoring that was needed. The rest are more subtle.

So for consistency, cache_entry_create_json() and cache_entry_set_json() now only take valid input (i.e. non-NULL json objects). The raw equivalents can take NULL & 0, b/c that's just
a zero length blobref.

New functions cache_entry_create_json_empty() and cache_entry_create_raw_empty() were created to create "empty" entries.

In order to clear data from an entry, cache_entry_clear_data() was added. The cache_entry_set_json() and cache_entry_set_raw() functions can no longer clear data from an entry by passing in NULL.

As a consequence, it's worth noting that cache_entry_set_raw() will also call wait_runqueue() when a zero-byte length buffer is set into the entry. This was not done before.

Debate on implementation:

Tiny part of me dislikes the function name "empty". Can discuss alternates. I considered using a flag type variable to say "I have data", but that seemed even uglier and more confusing.

I also considered removing the "empty" functions and supporting a cache_entry_set_type() function, to say these cache entries only support json or raw (i.e. user would call cache_entry_create(), then cache_entry_set_type()). But that's two function calls instead of one. I nixed that idea.

This comment has been minimized.

I'm going to be travelling tomorrow so I may not get to review this for a bit. One quick question though, do the cache_create() functions really need to be typed? I.e. does an invalid cache entry need a type?

This comment has been minimized.

Are you referring to the fact I have a "NONE" data type? I suppose this is mostly for internal reasons, to signify the user has not yet assigned "JSON" or "RAW" data, so the cache entry doesn't know. I suppose that could be masked/hidden internally.

Thought, is "type" perhaps the wrong word to use?

This comment has been minimized.

edited

It occurred to me last night that instead of creating cache_entry_create_json_empty() and cache_entry_create_raw_empty(), I could have modified cache_entry_create() to always create an "empty" entry, but the user can now pass in a data type to it. So

cache_entry_create(NONE) - means I don't know what this entry will holdcache_entry_create(JSON) - says I want this cache entry for json, but I don't have data yetcache_entry_create(RAW) - says I want this cache entry for raw, but I don't have data yet

probably better than adding two new functions.

Well, I'll let the review happen on what's there for now. I can change this later.

Also, "NONE" maybe should be changed to "UNKNOWN" ... TBD

This comment has been minimized.

edited

I was just thinking that one creation function could set the type to none/unknown, and it could get its type assigned when it becomes valid. Maybe that doesn't fit - I didn't look too closely. Sorry, getting packed up today so I'll be offline most of today, but I'll try to check in later.

This comment has been minimized.

As for your question, "it could get its type assigned when it becomes valid". Basically assigning a type (or none) is an option for the caller. In some cases, assigning the type ahead of time is effectively a flag saying, "Hey, this is what I'm expecting". I use this "flag" so that content-loading is aware what to expect.

cache_entry_create() now takes a cache_data_type_t parameter,
allowing to user to create a cache entry without any data
attached to it, but at the same time indicate the type of data
expected for the cache entry to store. This is an alternate
to passing NULL to the function cache_entry_create_json ().

Cache entry creation API has slightly different logic now. Instead
of creating an "empty" entry by setting inputs to NULL
(i.e. cache_entry_create_json(NULL) or cache_entry_create_raw(NULL, 0))
these create functions now expect correct input.
Callers wishing to create empty entries without data shall call
cache_entry_create().
As a side effect, this now means that cache_entry_create_raw() now
is not ambiguous about setting zero length data. Setting
cache_entry_create_raw(NULL, 0) creates a cache entry with a zero
length of data. Where as before, it was ambiguous if it was
an empty entry or an entry with zero length of data.
Update callers of these functions.
Update/add unit tests.

Alter usage of cache_entry_set_json(). If user wishes to clear
data, the user should use cache_entry_clear_data(). A NULL input
to cache_entry_set_json() will now result in an error.
Update code accordingly for change.

In the internal cache API, add a data_valid flag which indicates
if the user has set data or not. This flag is used instead of
checking data == NULL for validity, as checking the data pointer
for NULL would not allow zero length content-blobs to be stored
in the cache.
A major fallout of from this addition is the change to the
cache_entry_set_raw() function. It was previously unclear
if passing NULL & length 0 to cache_entry_set_raw() indicated
if you were setting a zero length data or trying to clear
prior data. Now it is made clear that this is setting zero
length data and clearing data is through cache_entry_clear_data().
In addition, calling cache_entry_set_raw() with zero length
data can result in wait_runqueue() being run on hp->waitlist_valid.
Added new unit tests for coverage.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.