hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Colin Patrick McCabe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-3920) libwebdhfs code cleanup: string processing and using strerror consistently to handle all errors
Date Thu, 04 Oct 2012 18:33:48 GMT

    [ https://issues.apache.org/jira/browse/HDFS-3920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13469578#comment-13469578
] 

Colin Patrick McCabe commented on HDFS-3920:
--------------------------------------------

This patch didn't quite apply for me.  I think you might have to rebase.

Good job creating hdfs_strerror.  I think it will make printing error messages a lot easier.
 Just a few notes: you don't have to check for sys_errlist[x] == NULL; these strings can't
be NULL.

I also think that libhdfs will probably want to use hdfs_strerror.  Perhaps we should move
it to hadoop-hdfs-project/hadoop-hdfs/src/main/native/util/posix_util.c.  It will be useful
on any POSIX operating system, so I think it belongs there.

I notice a few places where you are setting {{errno}} and then calling another function such
as {{fprintf}}.  Unfortunately, the {{fprintf}} function might change the value of {{errno}}.
 This mistake is unfortunately very easy to make even if you know about this problem.  This
is one reason why I strongly recommend the "de-errnoificiation" of APIs.

For example if you have an API that returns a struct Foo*, but could have an error, it's usually
better to do this:
{code}
int doStuff(struct Foo **foo)  __attribute__ ((warn_unused_result));
{code}

Than to do this:
{code}
struct Foo* doStuff(void); // sets errno on error
{code}

The second, errno-based one may *look* simpler, but danger lurks.  It's very easy for the
implementor to forget to set {{errno}} correctly somewhere in doStuff, or to call one of the
many functions which overwrite it.  The caller may also forget to check for NULL, or overwrite
the {{errno}} value before checking it.

So I would really recommend that you get rid of all the places where you're setting errno
to increase maintainability in the future.  (except the places where you're forced to because
you're implementing the libhdfs API.)

I also find it really confusing that "Response" is actually a pointer to a structure.  I do
not expect something to be a pointer unless it has the "*".  This is very important because
it makes it confusing to read code which appears to be copying structures, but is actually
just passing around pointers to them.  Can we just make Response a normal structure type,
rather than a typedef?  I don't think hitting the * key is an undue hardship, and it would
make the code so much more readable.  Same thing goes for ResponseBuffer-- just make it a
regular old structure, and you won't need ResponseBufferInternal at all.  typedefs are kind
of like macros-- you should use them very sparingly and only when there's a really good reason.

Overall, thanks for doing this cleanup.  I especially like the new comments.
                
> libwebdhfs code cleanup: string processing and using strerror consistently to handle
all errors
> -----------------------------------------------------------------------------------------------
>
>                 Key: HDFS-3920
>                 URL: https://issues.apache.org/jira/browse/HDFS-3920
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>            Reporter: Jing Zhao
>            Assignee: Jing Zhao
>         Attachments: HDFS-3920-001.patch, HDFS-3920-001.patch, HDFS-3920-002.patch, HDFS-3920-003.patch,
HDFS-3920-004.patch, HDFS-3920-005.patch
>
>
> 1. Clean up code for string processing;
> 2. Using strerror consistently for error handling;
> 3. Use sprintf to replace decToOctal
> 4. other issues requiring fixing.

--
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