Re: [Linuxptp-devel] [PATCH 2/2] hwtstamp_ctl: use SIOCGHWTSTAMP ioctl before destructively setting policy

[Keller, J. E <jac...@in...>]

On Mon, 2014-06-02 at 17:47 +0200, Richard Cochran wrote:
> Looks good. I have a few nits to pick...
> 
> On Tue, May 27, 2014 at 10:34:36AM -0700, Jacob Keller wrote:
> ...
> 
> > diff --git a/hwstamp_ctl.8 b/hwstamp_ctl.8
> > index cece74f61866..7bfdf4451839 100644
> > --- a/hwstamp_ctl.8
> > +++ b/hwstamp_ctl.8
> > @@ -15,11 +15,10 @@ hwstamp_ctl \- set time stamping policy at the driver level
> >  
> >  .SH DESCRIPTION
> >  .B hwstamp_ctl
> > -is a program used to set the hardware time stamping policy at the network
> > +is a program used to set and get the hardware time stamping policy at the network
> >  driver level with the
> >  .B SIOCSHWTSTAMP
> >  .BR ioctl (2).
> > -The
> 
> I think this sentence is more readable with the "The" at the beginning.

Oh, I see, I don't think I meant to drop that word, so I'll respin that.

Thanks,
Jake

> >  .I tx-type
> >  and
> >  .I rx-filter
> 
> ...
> 
> > @@ -138,15 +141,43 @@ int main(int argc, char *argv[])
> >  		return -1;
> >  	}
> >  
> > -	err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
> > +	/* First, attempt to get the current settings. */
> > +	err = ioctl(fd, SIOCGHWTSTAMP, &ifreq);
> >  	if (err < 0) {
> >  		err = errno;
> > -		perror("SIOCSHWTSTAMP failed");
> > -		if (err == ERANGE)
> > -			fprintf(stderr, "The requested time stamping mode is not supported by the hardware.\n");
> > +		if (err == ENOTTY)
> > +			fprintf(stderr, "Kernel does not have support for non-destructive for SIOCGHWTSTAMP.\n");
> 
> One "for" too many?

Hmm, ya..

> 
> Also the lines with the three error messages are really very long.
> 
> > +		else if (err == EOPNOTSUPP)
> > +			fprintf(stderr, "Device driver does not have support for non-destructive SIOCGHWTSTAMP.\n");
> > +		else
> > +			perror("SIOCGHWTSTAMP failed");
> > +	} else {
> > +		printf("current settings:\n" "tx_type %d\n" "rx_filter %d\n",
> > +		       cfg.tx_type, cfg.rx_filter);
> >  	}
> >  
> > -	printf("tx_type %d\n" "rx_filter %d\n", cfg.tx_type, cfg.rx_filter);
> > +	/* Now, attempt to set values. Only change the values actually
> > +	 * requested by user, rather than blindly resetting th zero if
> > +	 * unrequested. */
> > +	if (settx || setrx) {
> > +
> > +		if (settx)
> > +			cfg.tx_type = txopt;
> > +
> > +		if (setrx)
> > +			cfg.rx_filter = rxopt;
> > +
> > +		err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
> > +		if (err < 0) {
> > +			err = errno;
> > +			perror("SIOCSHWTSTAMP failed");
> > +			if (err == ERANGE)
> > +				fprintf(stderr, "The requested time stamping mode is not supported by the hardware.\n");
> 
> Very long.
> 

Too long of a line? I usually try to follow the Linux practice which
allows long lines if they only contain print statement, so that it's
easier to grep the source for where the error came from..

I can change it though. I don't think the message itself is too long
though.

> > +		}
> > +
> > +		printf("new settings:\n" "tx_type %d\n" "rx_filter %d\n", cfg.tx_type, cfg.rx_filter);
> 

I can break these onto their own lines, I suppose.

> Also too long.
> 

Regards,
Jake

> Thanks,
> Richard