[DTrace-devel] RESEND Re: [PATCH 1/2] Minor code cleanup

[Eugene Loh]

On 08/24/2020 06:51 AM, Kris Van Hees wrote:

> On Tue, Aug 11, 2020 at 03:28:11PM -0700, Eugene Loh wrote:
>> On 07/23/2020 02:07 PM, eugene.loh at oracle.com wrote:
>>> From: Eugene Loh <eugene.loh at oracle.com>
>>>
>>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
>
> ... with incorporating the comments below...

Got it.  I incorporated your comments and will push.

FWIW, however, I do not understand them.  There are minor, arguably 
related changes that may or may not be made as part of a patch, subject 
to some judgment call regarding what is in scope of the patch and what 
is not.  You argue that eliminating unneeded "(void)" is within scope in 
two cases, even though those "(void)" are a common practice throughout 
our code base and certainly within that file.  So, it would seem to me 
that eliminating that "(void)" practice would be its own patch, rather 
than just sticking two instances into this patch.  If we do not like 
that "(void)" style, we should purge it systematically.

In contrast, "cleaning up comments" would be hard to imagine as a 
single, coherent patch.  Typically, egregious comments would be judged 
on a case-by-case basis.  Cleaning up one such comment as part of a 
patch that is touching associated code would strike me as "in scope."

Not a big deal.  I changed it as you ask for the sake of getting these 
patches completed.  Ideally, however, it'd be nice if there were some 
way to think about this that made sense to us all.

>>> ---
>>>    libdtrace/dt_printf.c | 14 ++++++--------
>>>    1 file changed, 6 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/libdtrace/dt_printf.c b/libdtrace/dt_printf.c
>>> index 03ffa8bd..4382c4bf 100644
>>> --- a/libdtrace/dt_printf.c
>>> +++ b/libdtrace/dt_printf.c
>>> @@ -1549,7 +1549,7 @@ dtrace_freopen(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
>>>        const dtrace_probedata_t *data, const dtrace_recdesc_t *recp,
>>>        uint_t nrecs, const void *buf, size_t len)
>>>    {
>>> -	char selfbuf[40], restorebuf[40];
>>> +	char tmpbuf[40];
> OK
>
>>>    	FILE *nfp;
>>>    	int rval, errval;
>>>    	dt_pfargv_t *pfv = fmtdata;
>>> @@ -1580,11 +1580,10 @@ dtrace_freopen(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
>>>    			return (rval);
>>>    		}
>>>    
>>> -		(void) snprintf(restorebuf, sizeof (restorebuf),
>>> +		(void) snprintf(tmpbuf, sizeof (tmpbuf),
>>>    		    "/dev/fd/%d", dtp->dt_stdout_fd);
> Drop the "(void) " since that isn't necessary.
>
>>>    		free(dtp->dt_freopen_filename);
>>> -		dtp->dt_freopen_filename = strndup(restorebuf,
>>> -						   sizeof(restorebuf));
>>> +		dtp->dt_freopen_filename = strndup(tmpbuf, sizeof(tmpbuf));
> OK
>
>>>    	} else {
>>>    		free(dtp->dt_freopen_filename);
>>>    		dtp->dt_freopen_filename = strdup(dtp->dt_sprintf_buf);
>>> @@ -1593,8 +1592,7 @@ dtrace_freopen(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
>>>    	/*
>>>    	 * freopen(3C) will always close the specified stream and underlying
>>>    	 * file descriptor -- even if the specified file can't be opened.
>>> -	 * Even for the semantic cesspool that is standard I/O, this is
>>> -	 * surprisingly brain-dead behavior:  it means that any failure to
>>> +	 * This means that any failure to
> While I agree that the original authors could have been a bit more polite in
> their phrasing, I don't think changing it as part of this patch makes sense.
> If we want to sanitize the comments in the code, we should do so in patches
> that are specifically written for that.
>
>>>    	 * open the specified file destroys the specified stream in the
>>>    	 * process -- which is particularly relevant when the specified stream
>>>    	 * happens (or rather, happened) to be stdout.  This could be resolved
>>> @@ -1620,7 +1618,7 @@ dtrace_freopen(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
>>>    		return (errval);
>>>    	}
>>>    
>>> -	(void) snprintf(selfbuf, sizeof (selfbuf), "/dev/fd/%d", fileno(nfp));
>>> +	(void) snprintf(tmpbuf, sizeof (tmpbuf), "/dev/fd/%d", fileno(nfp));
> Drop the "(void) " since that isn't necessary.
>
>>>    
>>>    	if (dtp->dt_stdout_fd == -1) {
>>>    		/*
>>> @@ -1635,7 +1633,7 @@ dtrace_freopen(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
>>>    		}
>>>    	}
>>>    
>>> -	if (freopen(selfbuf, "aF", fp) == NULL) {
>>> +	if (freopen(tmpbuf, "aF", fp) == NULL) {
>>>    		(void) fclose(nfp);
>>>    		return (dt_set_errno(dtp, errno));
>>>    	}
>>
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel at oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel