Better error reporting in nacl helper.

Better error reporting in nacl helper.

BUG= https://code.google.com/p/chromium/issues/detail?id=136793
TEST= none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146103

[Mark Seaborn, 2012-07-09 09:19:05]

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:237: if (msglen > 0) {
">=" would be more appropriate than ">" since errno is only set on error, when
msglen < 0.

BTW, this code path would be clearer as:

if (msglen == 0 || (msglen == -1 && errno == ECONNRESET)) {
  ...
} else if (msglen < 0) {
  ...
} else if (msglen == sizeof...) {
  ...
} else {
  ...
}

i.e. known errors, unknown errors, known messages, unknown messages.

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:239: << std::string(buf, buf + msglen);
Is this appropriate, given that the buffer might contain binary data?  If the
buffer contains all zeros (for example), this won't help much.  BTW, is there a
bug filed for the problem you are trying to track down?

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:242: LOG(ERROR) << "receive from zygote failed,
errno = " << errno;
Prefix this with "nacl_helper:"?

[Mark Seaborn, 2012-07-09 09:19:05]

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:237: if (msglen > 0) {
">=" would be more appropriate than ">" since errno is only set on error, when
msglen < 0.

BTW, this code path would be clearer as:

if (msglen == 0 || (msglen == -1 && errno == ECONNRESET)) {
  ...
} else if (msglen < 0) {
  ...
} else if (msglen == sizeof...) {
  ...
} else {
  ...
}

i.e. known errors, unknown errors, known messages, unknown messages.

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:239: << std::string(buf, buf + msglen);
Is this appropriate, given that the buffer might contain binary data?  If the
buffer contains all zeros (for example), this won't help much.  BTW, is there a
bug filed for the problem you are trying to track down?

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:242: LOG(ERROR) << "receive from zygote failed,
errno = " << errno;
Prefix this with "nacl_helper:"?

[halyavin, 2012-07-09 10:55:56]

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:237: if (msglen > 0) {
On 2012/07/09 09:19:05, Mark Seaborn wrote:
> ">=" would be more appropriate than ">" since errno is only set on error, when
> msglen < 0.
> 
> BTW, this code path would be clearer as:
> 
> if (msglen == 0 || (msglen == -1 && errno == ECONNRESET)) {
>   ...
> } else if (msglen < 0) {
>   ...
> } else if (msglen == sizeof...) {
>   ...
> } else {
>   ...
> }
> 
> i.e. known errors, unknown errors, known messages, unknown messages.

Done.

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:239: << std::string(buf, buf + msglen);
On 2012/07/09 09:19:05, Mark Seaborn wrote:
> Is this appropriate, given that the buffer might contain binary data?  If the
> buffer contains all zeros (for example), this won't help much.  BTW, is there
a
> bug filed for the problem you are trying to track down?
We will print zeros then. They will not be visible to the user, but we can ask
user to redirect output to the file and then see binary content using hex mode
in Midnight Commander.

There is bug filed. The problem with NaCl was reported by external user in NaCl
discuss. Do you think we should create one?

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:242: LOG(ERROR) << "receive from zygote failed,
errno = " << errno;
On 2012/07/09 09:19:05, Mark Seaborn wrote:
> Prefix this with "nacl_helper:"?

Done.

[Mark Seaborn, 2012-07-10 16:10:05]

LGTM, with changes below if possible, and thanks.

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/10695077/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:239: << std::string(buf, buf + msglen);
On 2012/07/09 10:55:56, halyavin wrote:
> On 2012/07/09 09:19:05, Mark Seaborn wrote:
> > Is this appropriate, given that the buffer might contain binary data?  If
the
> > buffer contains all zeros (for example), this won't help much.  BTW, is
> > there a bug filed for the problem you are trying to track down?
> We will print zeros then. They will not be visible to the user, but we can ask
> user to redirect output to the file and then see binary content using hex mode
> in Midnight Commander.

That's kind of icky.  Maybe you could use GetDoubleQuotedJson() from
base/json/string_escape.h?

> There is bug filed. The problem with NaCl was reported by external user in
> NaCl discuss. Do you think we should create one?

Yes, please file one referencing the thread & error, and link it from the commit
message.

[commit-bot: I haz the power, 2012-07-11 10:17:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/halyavin@google.com/10695077/4

[commit-bot: I haz the power, 2012-07-11 11:33:20]

Change committed as 146103