Fixes #413: API for getting bucket and file by name #414
Conversation
src/utils.c
Outdated
|
|
||
| // count the number of replacements needed | ||
| ins = subject; | ||
| for (count = 0; tmp = strstr(ins, search); ++count) { |
There was a problem hiding this comment.
does this break when tmp == NULL?
There was a problem hiding this comment.
utils.c:104:25: warning: using the result of an assignment as a condition
without parentheses [-Wparentheses]
for (count = 0; tmp = strstr(ins, search); ++count) {
~~~~^~~~~~~~~~~~~~~~~~~~~
utils.c:104:25: note: place parentheses around the assignment to silence this
warning
for (count = 0; tmp = strstr(ins, search); ++count) {
^
( )
utils.c:104:25: note: use '==' to turn this assignment into an equality
comparison
for (count = 0; tmp = strstr(ins, search); ++count) {
^
==
1 warning generated.
There was a problem hiding this comment.
Yes. It breaks on tmp == NULL. I can rework this line to avoid the compiler warning.
|
update to handle variable freeing and returns with goto like other places in library. https://github.com/Storj/libstorj/blob/master/src/crypto.c#L134 |
|
Do you mean in the |
src/storj.c
Outdated
| return; | ||
| } | ||
|
|
||
| free(bucket_key_as_str); |
There was a problem hiding this comment.
this variable never gets freed when returning on line 259. handle frees and return using goto at end of function
There was a problem hiding this comment.
OK. I see it now. However, the same issues exists on many other places in the code.
I would suggest to move the code for generating the bucket key in a separate function that handles the memory correctly and replace it all over. Rather than polluting the code with gotos. Sounds good?
There was a problem hiding this comment.
we've been using gotos across the entire library. You could just put variables you need access to outside on the heap and the rest on the stack. I don't exactly understand what you are saying. maybe show me an example of what you mean
There was a problem hiding this comment.
Examples of exactly the same code with the same issue:
https://github.com/Storj/libstorj/blob/master/src/storj.c#L25-L37
https://github.com/Storj/libstorj/blob/master/src/storj.c#L111-L123
https://github.com/Storj/libstorj/blob/master/src/storj.c#L189-L201
https://github.com/Storj/libstorj/blob/master/src/storj.c#L267-L279
https://github.com/Storj/libstorj/blob/master/src/storj.c#L357-L369
|
Looks awesome to me. Tests passed on my local machine. No compiler warnings |
src/storj.c
Outdated
| return; | ||
| } | ||
|
|
||
| free(escaped_encrypted_file_name); |
There was a problem hiding this comment.
This may not get freed, could use goto with cleanup label
There was a problem hiding this comment.
I pushed 5c40693 that does this.
| @@ -80,6 +80,54 @@ char *str_concat_many(int count, ...) | |||
| return combined; | |||
| } | |||
|
|
|||
| char *str_replace(char *search, char *replace, char *subject) { | |||
There was a problem hiding this comment.
a few unit tests around this could be useful?
There was a problem hiding this comment.
Just added a unit test - see the latest commit.
|
So far looks good to me. Ran valgrind on it and didn't see any issues that are not already on the master branch. |
No description provided.