search

basho-labs / riak-c-client

Riak C Client
Apache License 2.0
20 stars 8 forks source link

Add Secondary Index Message #33

Closed hazen closed 10 years ago

broach commented 10 years ago

There were two issues here.

First, only the value portion of the pair was being copied.

Second, the RpbPair structs weren't being initialized, which would cause the client to core during PB serialization. This would affect setting indexes, links, or user metadata on a riak_object.

The other concern I have is that these repeating (RpbPair**) fields aren't being freed.

In riak_put.c/riak_put_request_encode() the RpbContent is created on the stack, then we're allocating the RpbPair** arrays and assigning them to RpbContent.indexes ... how are those ever freed? The RpbPutReq containing the content is packed into the buffer then is just destroyed when the stack frame gets nuked.

I'm almost thinking we should be mallocing these and then calling the *__free_unpacked() functions on them after packing them to the buffer.

broach commented 10 years ago

Ugh, so ... right. You can't do that, since you're just copying the pointers. You have to free the riak_object itself after the put request has completed.

hazen commented 10 years ago

So the memory allocated in a request is just pointers, not copied, until the PB binary has been created. Then the request data should be freed in the encode() functions.

We can chat to clarify my evil plan. Now if the code does what I want it to do is another question.

broach commented 10 years ago

The request data is not freed in the encode() function, and honestly it really can't be the way this is done.

The original riak_pair** and its riak_pair structs are malloced and the pointer is copied to the riak_object. You copy the pointers all the way down into the PB message. If you free those in the encode function, back up in the call stack the user now has a mucked up riak_object. And if they then free the riak object ... bad things. (*Unless I'm missing where the memory is actually copied vs. the pointers somewhere)

hazen commented 10 years ago

Right. The incoming riak_pair should be freed by the user. The RpbIndexReq, for example, is a temporary object.

hazen commented 10 years ago

@broach I created an issue for cleaning up the riak_pair interface on the put message https://github.com/basho/riak-c-client/issues/39. Some of those original messages need to be cleaned up anyway. I also created an issue on the internal print interface https://github.com/basho/riak-c-client/issues/40.

As far as the 2i implementation goes, are you OK with merging this current PR?

broach commented 10 years ago

:+1: for this PR