[
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
