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

[Richard C. <ric...@gm...>]

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.

>  .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?

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.

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

Also too long.

Thanks,
Richard