[update_engine] handle NULL returns from strdup

[update_engine] handle NULL returns from strdup

grepped for calls to strdup, added code to handle NULL gracefully.

BUG=chromium-os:6589
TEST=unit tests

Change-Id: I95f07d591177711351abe268f88dbe0e24d39637

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=c6c57a5

[petkov, 2010-09-23 16:34:54]

Some nits and questions...

http://codereview.chromium.org/3437019/diff/1/2
File dbus_service.cc (right):

http://codereview.chromium.org/3437019/diff/1/2#newcode80
dbus_service.cc:80: gboolean alloc_successful = *current_operation &&
*new_version;
Do you need to set *error to NULL if alloc_successful is FALSE? Of course,
ideally, you'd be setting error to something meaningful but I don't think it
matters in this case.

http://codereview.chromium.org/3437019/diff/1/3
File subprocess.cc (right):

http://codereview.chromium.org/3437019/diff/1/3#newcode43
subprocess.cc:43: int FreeArgvInError(char** argv) {
add an empty line before function.

http://codereview.chromium.org/3437019/diff/1/3#newcode46
subprocess.cc:46: return 0;
What's the return value for? Why not make this function void and return the
appropriate status from callers?

http://codereview.chromium.org/3437019/diff/1/3#newcode70
subprocess.cc:70: for (--pointer; pointer >= 0; --pointer)
Isn't this equivalent to FreeArgv(ret)? Maybe rename FreeArgv and reuse?

[Chris Masone, 2010-09-23 17:46:05]

http://codereview.chromium.org/3437019/diff/1/2
File dbus_service.cc (right):

http://codereview.chromium.org/3437019/diff/1/2#newcode80
dbus_service.cc:80: gboolean alloc_successful = *current_operation &&
*new_version;
On 2010/09/23 16:34:54, petkov wrote:
> Do you need to set *error to NULL if alloc_successful is FALSE? Of course,
> ideally, you'd be setting error to something meaningful but I don't think it
> matters in this case.
> 
> 

Done.

http://codereview.chromium.org/3437019/diff/1/3
File subprocess.cc (right):

http://codereview.chromium.org/3437019/diff/1/3#newcode43
subprocess.cc:43: int FreeArgvInError(char** argv) {
On 2010/09/23 16:34:54, petkov wrote:
> add an empty line before function.
> 

Done.

http://codereview.chromium.org/3437019/diff/1/3#newcode46
subprocess.cc:46: return 0;
On 2010/09/23 16:34:54, petkov wrote:
> What's the return value for? Why not make this function void and return the
> appropriate status from callers?
> 
> 

So I can just call this and return the value.  Otherwise it makes the points of
call a lot less readable.

http://codereview.chromium.org/3437019/diff/1/3#newcode70
subprocess.cc:70: for (--pointer; pointer >= 0; --pointer)
On 2010/09/23 16:34:54, petkov wrote:
> Isn't this equivalent to FreeArgv(ret)? Maybe rename FreeArgv and reuse?
> 

yes, I believe it is the same, as long as ret is NULL terminated no matter what.
 And I think it needs to be, so I'll just enforce that and then use FreeArgv.

[adlr, 2010-09-23 18:21:21]

i guess i'm w/ darren on this on. i don't see how adding 'return 0' less
readable

http://codereview.chromium.org/3437019/diff/6001/7002
File subprocess.cc (right):

http://codereview.chromium.org/3437019/diff/6001/7002#newcode44
subprocess.cc:44: int FreeArgvInError(char** argv) {
it's a little weird for this to return an int, which is a glib tag. How about
making this void and making it's callers call "return 0;" after calling this

[petkov, 2010-09-23 18:27:10]

http://codereview.chromium.org/3437019/diff/6001/7001
File dbus_service.cc (right):

http://codereview.chromium.org/3437019/diff/6001/7001#newcode80
dbus_service.cc:80: if (!(*current_operation && *new_version)) {
I usually find if (!a || !b) a bit more readable but I think readability is in
the eyes of the beholder in this case so OK...

http://codereview.chromium.org/3437019/diff/6001/7002
File subprocess.cc (right):

http://codereview.chromium.org/3437019/diff/6001/7002#newcode57
subprocess.cc:57: memset(ret, 0, (arraysize(keys) + 1) * sizeof(char*));
Alternatively, you can do ret[pointer] = NULL; right before "return ret" at the
end.

http://codereview.chromium.org/3437019/diff/6001/7002#newcode66
subprocess.cc:66: ret = NULL;
You may as well just return NULL here.

[Chris Masone, 2010-09-23 19:40:23]

http://codereview.chromium.org/3437019/diff/6001/7002
File subprocess.cc (right):

http://codereview.chromium.org/3437019/diff/6001/7002#newcode44
subprocess.cc:44: int FreeArgvInError(char** argv) {
On 2010/09/23 18:21:21, adlr wrote:
> it's a little weird for this to return an int, which is a glib tag. How about
> making this void and making it's callers call "return 0;" after calling this

Done.

http://codereview.chromium.org/3437019/diff/6001/7002#newcode57
subprocess.cc:57: memset(ret, 0, (arraysize(keys) + 1) * sizeof(char*));
On 2010/09/23 18:27:10, petkov wrote:
> Alternatively, you can do ret[pointer] = NULL; right before "return ret" at
the
> end.
> 

Done.

http://codereview.chromium.org/3437019/diff/6001/7002#newcode66
subprocess.cc:66: ret = NULL;
On 2010/09/23 18:27:10, petkov wrote:
> You may as well just return NULL here.
> 

Done.

[petkov, 2010-09-23 19:55:21]

LGTM w/ more nits.

http://codereview.chromium.org/3437019/diff/13001/2003
File subprocess.cc (right):

http://codereview.chromium.org/3437019/diff/13001/2003#newcode146
subprocess.cc:146: return 0;
return false

http://codereview.chromium.org/3437019/diff/13001/2003#newcode154
subprocess.cc:154: return 0;
return false

[Chris Masone, 2010-09-23 20:03:08]

pushing...

http://codereview.chromium.org/3437019/diff/13001/2003
File subprocess.cc (right):

http://codereview.chromium.org/3437019/diff/13001/2003#newcode146
subprocess.cc:146: return 0;
On 2010/09/23 19:55:22, petkov wrote:
> return false
> 

Done.

http://codereview.chromium.org/3437019/diff/13001/2003#newcode154
subprocess.cc:154: return 0;
On 2010/09/23 19:55:22, petkov wrote:
> return false
> 

Done.