hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andy Isaacson (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-3916) libwebhdfs (C client) code cleanups
Date Tue, 11 Sep 2012 20:16:08 GMT

    [ 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

Mime
View raw message