[
https://issues.apache.org/jira/browse/HDFS-3916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13453358#comment-13453358
]
Andy Isaacson commented on HDFS-3916:
-------------------------------------
Thank you for the cleanup patch, Colin!
A few other issues you may want to address, sorry that this reads basically
like a post-hoc code review of HDFS-2656. And, apologies if I cite any issues
you've addressed in the attached patch, I'm just reading SVN commit 1382836.
* nearly all uses of strcat should be replaced by strncat or similar bounded
copy routines.
* {{sizeof(char)}} is defined to be 1, so code like {{u = malloc((uriLen + 1) *
(sizeof(char)));}} should be {{u = malloc(uriLen++ 1);}}
{code}
+int checkIfRedirect(const char *const headerstr, const char *content, const
char *operation) {
...
+ if(headerstr == '\0' || strncmp(headerstr,"HTTP/", 5)) {
{code}
Comparing a pointer {{headerstr}} against the character constant {{'\0'}} is
probably wrong. I can't tell if the coder intended to handle {{headerstr ==
NULL}} or {{headerstr[0] == '\0'}}.
{code}
+ while (DnLocation && strncmp(DnLocation, "Location:",
strlen("Location:"))) {
+ DnLocation = strtok_r(NULL, delims, &savepter);
+ }
+ if (!DnLocation) {
+ return NULL;
+ }
+ DnLocation = strstr(DnLocation, "http");
{code}
Using strstr here will not correctly handle a URL like "ftp://http.foo.com";.
It should skip whitespace and check the beginning of the word for "http", or
something like that.
{code}
+ des = pthread_cond_destroy(&buffer->transfer_finish);
+ if (des == EBUSY) {
+ fprintf(stderr, "The condition transfer_finish is still
referenced!\n");
+ } else if (des == EINVAL) {
+ fprintf(stderr, "The condition transfer_finish is invalid!\n");
+ }
{code}
Please use strerror and consistently handle all errors, rather than silently
discarding unexpected error values.
{code}
+ newAddr[strlen(bld->nn) - strlen(lastColon)] = '\0';
{code}
That assignment is terrifying, it would be very easy for a nonobvious bug to
cause this calculation to end up outside of the buffer and overwrite other
memory. In general libwebhdfs has way too much "clever" (dangerous) string
processing code.
{code}
+ absPath = (char *)malloc(strlen(path) + 1);
{code}
No need to cast the return value of malloc in C code.
{code}
+int hdfsExists(hdfsFS fs, const char *path)
+{
+ hdfsFileInfo *fileInfo = hdfsGetPathInfo(fs, path);
+ if (fileInfo) {
+ hdfsFreeFileInfo(fileInfo, 1);
+ return 0;
+ } else {
+ return -1;
+ }
{code}
Boolean functions in C should return 0 for false and 1 for true. This returns
0 for true and -1 for false.
{code}
+static void *writeThreadOperation(void *v) {
+ threadData *data = (threadData *) v;
{code}
No need to cast from void* to threadData*.
{code}
+ fprintf(stderr, "Error (code %d) when pthread_join.\n", ret);
{code}
Please use strerror.
{code}
+ fprintf(stderr, "To clean the webfilehandle...\n");
{code}
Remove this debugging printf.
{code}
+ if (!hashTableInited) {
+ LOCK_HASH_TABLE();
+ if (!hashTableInited) {
+ if (hcreate(MAX_HASH_TABLE_ELEM) == 0) {
+ fprintf(stderr, "error creating hashtable, <%d>: %s\n",
+ errno, strerror(errno));
+ return 0;
{code}
This early exit leaves the LOCK_HASH_TABLE locked. I'm glad this fprintf uses
strerror, but the formatting is inconsistent hroughout the patch; we should
have a common way of formatting the error messages in libwebhdfs.
{code}
+ char jvmArgDelims[] = " ";
{code}
There are many examples of this pattern in the patch, where a local array copy
of a string is made unnecessarily (the array is never written to). Nearly
every initialized char array should be a string instead: {{const char
*jvmArgDelims = " ";}}.
{code}+ switch(permissionsId) {
+ case 7:
+ perm = "rwx"; break;
+ case 6:
+ perm = "rw-"; break;
+ case 5:
+ perm = "r-x"; break;
+ case 4:
+ perm = "r--"; break;
+ case 3:
+ perm = "-wx"; break;
+ case 2:
+ perm = "-w-"; break;
+ case 1:
+ perm = "--x"; break;
+ case 0:
+ perm = "---"; break;
+ default:
+ perm = "???";
+ }
+ strncpy(rtr, perm, 3);
{code}
This should be something like
{code}
int perm = permissions >> (i * 3);
rtr[0] = perm & 4 ? 'r' : '-';
rtr[1] = perm & 2 ? 'w' : '-';
rtr[2] = perm & 1 ? 'x' : '-';
{code}
{code}
+ exit(-1);
{code}
exit(1) is the normal way to indicate failure.
{code}
+ const char* writePath = "/tmp/testfile.txt";
{code}
Tests should generate a test-specific filename and should use TMPDIR
appropriately. mktemp(3) or similar is helpful.
{code}
+static int decToOctal(int decNo) {
+ int octNo=0;
+ int expo =0;
+ while (decNo != 0) {
+ octNo = ((decNo % 8) * pow(10,expo)) + octNo;
+ decNo = decNo / 8;
+ expo++;
+ }
+ return octNo;
+}
...
+ mode = decToOctal(mode);
+ sprintf(permission,"%d",mode);
{code}
This can be replaced with a simple {{sprintf(permission, "%o", mode);}}.
> libwebhdfs (C client) code cleanups
> -----------------------------------
>
> Key: HDFS-3916
> URL: https://issues.apache.org/jira/browse/HDFS-3916
> Project: Hadoop HDFS
> Issue Type: Bug
> Components: webhdfs
> Affects Versions: 2.0.3-alpha
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Attachments: 0002-fix.patch, HDFS-3916.003.patch, HDFS-3916.004.patch
>
>
> Code cleanups in libwebhdfs.
> * don't duplicate exception.c, exception.h, expect.h, jni_helper.c. We have
> one copy of these files; we don't need 2.
> * remember to set errno in all public library functions (this is part of the
> API)
> * fix undefined symbols (if a function is not implemented, it should return
> ENOTSUP, but still exist)
> * don't expose private data structures in the (end-user visible) public
> headers
> * can't re-use hdfsBuilder as hdfsFS, because the strings in hdfsBuilder are
> not dynamically allocated.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
