fixed DR-i#1486

fixed DR-i#1486
- provide better error message on Dr.Memory registration failure

R=bruening@google.com
BUG=DR-i#1486

Committed: https://code.google.com/p/drmemory/source/detail?r=2010

[bruening, 2014-08-06 23:19:47]

LGTM w/ suggestions

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c
File drmemory/frontend.c (right):

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode191
drmemory/frontend.c:191: dr_config_status_to_msg(dr_config_status_t status)
Do you think drconfiglib should provide this routine?  Maybe XXX or file issue
if don't want to think about it now.

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode205
drmemory/frontend.c:205: msg = "target process is not registerred";
spelling

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode213
drmemory/frontend.c:213: msg = "conflict id";
s/conflict/conflicting/

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode221
drmemory/frontend.c:221: msg = "target process is not under Dr.Memory";
s/Dr.M/Dr. M/

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode224
drmemory/frontend.c:224: /* Nudge operation timed out waiting for target process
to finish handling a nudge.*/
style

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode224
drmemory/frontend.c:224: /* Nudge operation timed out waiting for target process
to finish handling a nudge.*/
(IMHO these comments could all be removed from here, not adding much)

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode233
drmemory/frontend.c:233: msg = "failed to write to the config file";
Perhaps this should also say "check permissions on XXX" -- hmm do we have an
easy way to get name of config file or basedir?

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode241
drmemory/frontend.c:241: msg = "unknown failure, check caller permissions for
this operation";
"caller"?  Does the user know anything about "caller"?

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode1353
drmemory/frontend.c:1353: res = dr_nudge_pid(nudge_pid, DRMEM_CLIENT_ID,
NUDGE_LEAK_SCAN, INFINITE);
Is this a bug b/c "res" is the wrong type?

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode1589
drmemory/frontend.c:1589: fatal("%d (%s)\nFailed to register DynamoRIO
configuration for \"%s\"(%d) "
Hmm, so this will read sthg like "ERROR: 3 (failed to write to the config
file)\nFailed...".  Hmm.  OK.  But maybe better if it's more like the one above
that has the "failed to register..." first, and then has "error code %d (%s)"?

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode1601
drmemory/frontend.c:1601: "ops=\"%s\"", status, err_msg, client_path,
client_ops);
Ditto

[zhaoqin, 2014-08-08 19:18:04]

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c
File drmemory/frontend.c (right):

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode191
drmemory/frontend.c:191: dr_config_status_to_msg(dr_config_status_t status)
On 2014/08/06 23:19:46, bruening wrote:
> Do you think drconfiglib should provide this routine?  Maybe XXX or file issue
> if don't want to think about it now.

hmm, actually, I should add it to drconfiglib now.

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode205
drmemory/frontend.c:205: msg = "target process is not registerred";
On 2014/08/06 23:19:46, bruening wrote:
> spelling

Done.

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode213
drmemory/frontend.c:213: msg = "conflict id";
On 2014/08/06 23:19:46, bruening wrote:
> s/conflict/conflicting/

Done.

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode221
drmemory/frontend.c:221: msg = "target process is not under Dr.Memory";
On 2014/08/06 23:19:46, bruening wrote:
> s/Dr.M/Dr. M/

replace it with DynamoRIO since moving it to DynamoRIO, any client could use it.

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode224
drmemory/frontend.c:224: /* Nudge operation timed out waiting for target process
to finish handling a nudge.*/
On 2014/08/06 23:19:46, bruening wrote:
> style

Done.

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode224
drmemory/frontend.c:224: /* Nudge operation timed out waiting for target process
to finish handling a nudge.*/
On 2014/08/06 23:19:46, bruening wrote:
> (IMHO these comments could all be removed from here, not adding much)

Done.

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode233
drmemory/frontend.c:233: msg = "failed to write to the config file";
On 2014/08/06 23:19:46, bruening wrote:
> Perhaps this should also say "check permissions on XXX" -- hmm do we have an
> easy way to get name of config file or basedir?

For moving to DynamoRIO, I think we can only say "failed to write to the config
file".

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode241
drmemory/frontend.c:241: msg = "unknown failure, check caller permissions for
this operation";
On 2014/08/06 23:19:46, bruening wrote:
> "caller"?  Does the user know anything about "caller"?

just keep unknown error.

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode1353
drmemory/frontend.c:1353: res = dr_nudge_pid(nudge_pid, DRMEM_CLIENT_ID,
NUDGE_LEAK_SCAN, INFINITE);
On 2014/08/06 23:19:46, bruening wrote:
> Is this a bug b/c "res" is the wrong type?

good catch.

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode1589
drmemory/frontend.c:1589: fatal("%d (%s)\nFailed to register DynamoRIO
configuration for \"%s\"(%d) "
On 2014/08/06 23:19:46, bruening wrote:
> Hmm, so this will read sthg like "ERROR: 3 (failed to write to the config
> file)\nFailed...".  Hmm.  OK.  But maybe better if it's more like the one
above
> that has the "failed to register..." first, and then has "error code %d (%s)"?

ok, I used to put the error at the end, but the line is too long, kind of hard
to find the error code, add \n should solve the problem.

https://codereview.appspot.com/119520045/diff/1/drmemory/frontend.c#newcode1601
drmemory/frontend.c:1601: "ops=\"%s\"", status, err_msg, client_path,
client_ops);
On 2014/08/06 23:19:46, bruening wrote:
> Ditto

Done.

[zhaoqin, 2014-08-08 20:47:12]

Committed patchset #3 manually as 2010 (presubmit successful).