[jira] [Comment Edited] (HDFS-12340) Ozone: C/C++ implementation of ozone client using curl
[ https://issues.apache.org/jira/browse/HDFS-12340?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16247404#comment-16247404 ] Mukul Kumar Singh edited comment on HDFS-12340 at 11/10/17 12:10 PM: - Thanks for the updated patch [~shashikant] 1) #add_definitions(-DLIBOZONECLIENT_DLL_EXPORT), has been commented out from the second CMakeLists.txt, should this be removed ? 2) main.c - 19: a space between ozone and client 3) main.c - 135, memory has already been allocated at 118, that needs to be de-allocated 4) Comments are needed in main.c to explain the code flow. 5) main.c:142, memory is being malloc'ed here without any check, 6) main.c:144, there are commented lines of code 7) these series of elseif can be replcaced with switch cases and enums, and then lets have small helper functions for each command type. was (Author: msingh): 1) #add_definitions(-DLIBOZONECLIENT_DLL_EXPORT), has been commented out from the second CMakeLists.txt, should this be removed ? 2) main.c - 19: a space between ozone and client 3) main.c - 135, memory has already been allocated at 118, that needs to be de-allocated 4) Comments are needed in main.c to explain the code flow. 5) main.c:142, memory is being malloc'ed here without any check, 6) main.c:144, there are commented lines of code 7) these series of elseif can be replcaced with switch cases and enums, and then lets have small helper functions for each command type. > Ozone: C/C++ implementation of ozone client using curl > -- > > Key: HDFS-12340 > URL: https://issues.apache.org/jira/browse/HDFS-12340 > Project: Hadoop HDFS > Issue Type: Sub-task > Components: ozone >Affects Versions: HDFS-7240 >Reporter: Shashikant Banerjee >Assignee: Shashikant Banerjee > Labels: OzonePostMerge > Fix For: HDFS-7240 > > Attachments: HDFS-12340-HDFS-7240.001.patch, > HDFS-12340-HDFS-7240.002.patch, HDFS-12340-HDFS-7240.003.patch, main.C, > ozoneClient.C, ozoneClient.h > > > This Jira is introduced for implementation of ozone client in C/C++ using > curl library. > All these calls will make use of HTTP protocol and would require libcurl. The > libcurl API are referenced from here: > https://curl.haxx.se/libcurl/ > Additional details would be posted along with the patches. -- This message was sent by Atlassian JIRA (v6.4.14#64029) - To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (HDFS-12340) Ozone: C/C++ implementation of ozone client using curl
[
https://issues.apache.org/jira/browse/HDFS-12340?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16156526#comment-16156526
]
Anu Engineer edited comment on HDFS-12340 at 9/7/17 6:05 AM:
-
Thank you for updating the patch. Some more minor comments.
1. /hadoop-common/src/CMakeLists.txt: Can we please handle the case where
there is no curl installed on the build machine? if not found case.
2.nit: Can we please format the curl finding part consistent with the code
around it?
3. src/main/native/libozoneclient/CMakeLists.txt: nit: I find it unorthodox to
name the extension with a capital letter as in ".C", if there is such a
convention can you please share?
4. LIBOZONECLIENT_VERSION "0.0.0", can we please set to this to 0.0.1 ?
5. Same comment for other CMake files where we use a ".C" file convention.
6. In main.c -- Can we use a proper command parser instead of case 1: 2: etc. I
am looking for something like
{noformat}
> createVolume volume1
> help
{noformat}
etc. and correspondingly change the printUsage ?
7. In main.c -- since we have a bunch of isValidStr calls in the code, does it
make sense to put them in the same place and have one printUsage call ? also
would you like to init rc to -1 or 1 as the case may be and just return the rc
always?
8. In main.c: main function -- can you please init all the variables.
{noformat}
char user[64] = {0};
char serverIp[64] = {0};
char port[64] = {0};
char version[16] = {0};
char vol[256] = {0};
char bucket[256] = {0};
char infile[256] = {0};
char key[256] = {0};
{noformat}
9. ozone_client.C: nit: can we please rename this as ozone_client.c -- small
"c" as the name of the file.
10. we have isValidStr twice, I agree the scope is restricted, but I think we
can define it once and can be used with 0 to cover the first use case.
11. nit: int executeCurlDownloadOpearation(ozoneInfo* info, char *url, FILE
*fd); can we please have consistent placement for the * operator ? always with
the variable and away from the type ?
12: Again a nit: you don't have to fix this. When we write a library, it is
very rare that we will print to streams, even error stream. Generally, we will
return different error codes so that calling application can decide how to
print it, or we can provide a function that returns error string if needed. It
makes it easier to use the library under c/c++ code. Please see how curl itself
does this.
13: nit : you don't need these lines.
{noformat}
/* initialize the ozone Info structure */
info->curl = NULL;
info->chunk = NULL;
info->url = NULL;
info->user = NULL;
info->version = NULL;
{noformat}
calloc already takes care of this.
14: in the error path of initOzoneClient, don't we have to call the opposite
function of curl_easy_init, that is curl_easy_cleanup ?
15: In function initOzoneClient, the variable rc is not needed. You can jump to
exit only if you get a failure and return info just before that. That way you
can eliminate the if / else condition too.
16: In function resetOzoneInfo: nit: line 148, double semi-colon at the end.
17: Before we dereference info, check it is not null. Line 152
18: resetOzoneInfo: The error handling of this function seems wrong. Here is a
case where there could be a resource leak. Let us say the function fails at
line 158, where is trying to do curl_easy_init(). The function fails with
returning \-1. Now, what do I do with the ozoneInfo object? I cannot call the
resetOzoneInfo function safely, since curl_easy_cleanup will fail if I try to
call this function again, and I have no idea how to release this object. I
would suggest that we re-write it, possibly with better error handling
strategy. I cannot call destroyOzoneClient since it would suffer from the same
issue that curl_easy_cleanup would happen on the info->curl, which has not been
allocated.
19: In resetOzoneInfo: Is it possible that chunk will just accumulate the same
tags again and again ?
20: In createBucket:
{noformat}
strcpy(url, info->url);
strcat(url, volumeName);
strcat(url, "/");
strcat(url, bucketName);
{noformat}
You can replace this with a single call to sprintf, same comment every other
function.
21: In putKey: if we fail the if test in line 341/346/351/356 the program will
crash since we are closing a NULL file in the exit path.
was (Author: anu):
Thank you for updating the patch. Some more minor comments.
1. /hadoop-common/src/CMakeLists.txt: Can we please handle the case where
there is no curl installed on the build machine? if not found case.
2.nit: Can we please format the curl finding part consistent with the code
around it?
3. src/main/native/libozoneclient/CMakeLists.txt: nit: I find it unusable to
name the extension with a capital letter as in ".C", if there is such a
convention can you please share?
4. LIBOZONECLIENT_VERSION "0.0
[jira] [Comment Edited] (HDFS-12340) Ozone: C/C++ implementation of ozone client using curl
[
https://issues.apache.org/jira/browse/HDFS-12340?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16156526#comment-16156526
]
Anu Engineer edited comment on HDFS-12340 at 9/7/17 6:04 AM:
-
Thank you for updating the patch. Some more minor comments.
1. /hadoop-common/src/CMakeLists.txt: Can we please handle the case where
there is no curl installed on the build machine? if not found case.
2.nit: Can we please format the curl finding part consistent with the code
around it?
3. src/main/native/libozoneclient/CMakeLists.txt: nit: I find it unusable to
name the extension with a capital letter as in ".C", if there is such a
convention can you please share?
4. LIBOZONECLIENT_VERSION "0.0.0", can we please set to this to 0.0.1 ?
5. Same comment for other CMake files where we use a ".C" file convention.
6. In main.c -- Can we use a proper command parser instead of case 1: 2: etc. I
am looking for something like
{noformat}
> createVolume volume1
> help
{noformat}
etc. and correspondingly change the printUsage ?
7. In main.c -- since we have a bunch of isValidStr calls in the code, does it
make sense to put them in the same place and have one printUsage call ? also
would you like to init rc to -1 or 1 as the case may be and just return the rc
always?
8. In main.c: main function -- can you please init all the variables.
{noformat}
char user[64] = {0};
char serverIp[64] = {0};
char port[64] = {0};
char version[16] = {0};
char vol[256] = {0};
char bucket[256] = {0};
char infile[256] = {0};
char key[256] = {0};
{noformat}
9. ozone_client.C: nit: can we please rename this as ozone_client.c -- small
"c" as the name of the file.
10. we have isValidStr twice, I agree the scope is restricted, but I think we
can define it once and can be used with 0 to cover the first use case.
11. nit: int executeCurlDownloadOpearation(ozoneInfo* info, char *url, FILE
*fd); can we please have consistent placement for the * operator ? always with
the variable and away from the type ?
12: Again a nit: you don't have to fix this. When we write a library, it is
very rare that we will print to streams, even error stream. Generally, we will
return different error codes so that calling application can decide how to
print it, or we can provide a function that returns error string if needed. It
makes it easier to use the library under c/c++ code. Please see how curl itself
does this.
13: nit : you don't need these lines.
{noformat}
/* initialize the ozone Info structure */
info->curl = NULL;
info->chunk = NULL;
info->url = NULL;
info->user = NULL;
info->version = NULL;
{noformat}
calloc already takes care of this.
14: in the error path of initOzoneClient, don't we have to call the opposite
function of curl_easy_init, that is curl_easy_cleanup ?
15: In function initOzoneClient, the variable rc is not needed. You can jump to
exit only if you get a failure and return info just before that. That way you
can eliminate the if / else condition too.
16: In function resetOzoneInfo: nit: line 148, double semi-colon at the end.
17: Before we dereference info, check it is not null. Line 152
18: resetOzoneInfo: The error handling of this function seems wrong. Here is a
case where there could be a resource leak. Let us say the function fails at
line 158, where is trying to do curl_easy_init(). The function fails with
returning \-1. Now, what do I do with the ozoneInfo object? I cannot call the
resetOzoneInfo function safely, since curl_easy_cleanup will fail if I try to
call this function again, and I have no idea how to release this object. I
would suggest that we re-write it, possibly with better error handling
strategy. I cannot call destroyOzoneClient since it would suffer from the same
issue that curl_easy_cleanup would happen on the info->curl, which has not been
allocated.
19: In resetOzoneInfo: Is it possible that chunk will just accumulate the same
tags again and again ?
20: In createBucket:
{noformat}
strcpy(url, info->url);
strcat(url, volumeName);
strcat(url, "/");
strcat(url, bucketName);
{noformat}
You can replace this with a single call to sprintf, same comment every other
function.
21: In putKey: if we fail the if test in line 341/346/351/356 the program will
crash since we are closing a NULL file in the exit path.
was (Author: anu):
Thank you for updating the patch. Some more minor comments.
1. /hadoop-common/src/CMakeLists.txt: Can we please handle the case where
there is not curl installed on the build machine? if not found case.
2.nit: Can we please format the curl finding part consistent with the code
around it?
3. src/main/native/libozoneclient/CMakeLists.txt: nit: I find it unusable to
name the extension with a capital letter as in ".C", if there is such a
convention can you please share?
4. LIBOZONECLIENT_VERSION "0.0.
