[Bug 2687] Coverity scan fixes

[bugzilla-daemon at bugzilla.mindrot.org]

https://bugzilla.mindrot.org/show_bug.cgi?id=2687

--- Comment #7 from Jakub Jelen <jjelen at redhat.com> ---
Thanks for comments and thoughtful reread. Adding answers where it
makes sense.

(In reply to Darren Tucker from comment #4)
> Comment on attachment 2953 [details]
> > 			response = read_passphrase("Accept updated hostkeys? "
> > 			    "(yes/no): ", RP_ECHO);
> >-			if (strcasecmp(response, "yes") == 0)
> >+			if (response != NULL && strcasecmp(response, "yes") == 0)
> 
> I think this is a false positive.
> read_passphrase() can only return NULL if given the RP_ALLOW_EOF
> flag, otherwise the return values all come from xstrdup which will
> provide a valid pointer or die trying.

In that case it does not make sense to compare with NULL two lines
below (which was the concern of coverity report REVERSE_INULL):

>			else if (quit_pending || response == NULL ||

yes. My patch was one possible solution, but there are other (maybe
better) ways how to make it clear.

> > dump_cfg_string(ServerOpCodes code, const char *val)
> > {
> >-	if (val == NULL)
> >-		return;
> > 	printf("%s %s\n", lookup_opcode_name(code),
> > 	    val == NULL ? "none" : val);
> 
> not sure what the intent of this was, will need to investigate.

either the first or the second check for NULL is bogus. We should
prefer dumping configuration options with "none" rather than not, but
making sure that the options have empty value "none" accepted is a good
thing to consider before applying. 

(In reply to Darren Tucker from comment #6)
> Comment on attachment 2954 [details]
> 2nd part with lower priority
> >-			    filename, linenum, arg ? arg : "<NONE>");
> >+			    filename, linenum, arg);
> 
> I'm not sure removing this is a good idea; it might not be possible
> for arg to be null right now but later?

That is valid point. But now it is just a bogus check. I don't think
these days any of the options allow empty argument (and only case where
I left the <NONE> check was in (s|o)LogLevel and sLogFacility, which do
not have the check for missing argument in the first place.

It quite looked like copy&paste "error" from some other places.
Standardizing also the other mentioned case to test for missing
argument in first place would probably make sense too.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.