Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 21 Jan 2012 11:13:14 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Hiroki Sato <hrs@FreeBSD.org>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, eadler@FreeBSD.org
Subject:   Re: svn commit: r230354 - head/usr.sbin/makefs
Message-ID:  <20120121103348.Q1254@besplex.bde.org>
In-Reply-To: <20120120.123256.1432718473132856309.hrs@allbsd.org>
References:  <201201200138.q0K1cSou016739@svn.freebsd.org> <20120120.123256.1432718473132856309.hrs@allbsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 20 Jan 2012, Hiroki Sato wrote:

> Eitan Adler <eadler@FreeBSD.org> wrote
>  in <201201200138.q0K1cSou016739@svn.freebsd.org>:
>
> ea> Log:
> ea>   Fix a variety of warnings when compiling with gcc46
> ea>
> ea>   Approved by:	dim, cperciva (mentor, blanket for pre-mentorship already-approved commits)
> ea>   MFC after:	3 days
> ea>
> ea> Modified:
> ea>   head/usr.sbin/makefs/cd9660.c
>
> Removing the dot handling part and leaving a comment in lines
> 1106-1117 make people confused.
>
> In addition to that, I personally don't think this should be removed
> because our cd9660.c is still based on NetBSD's one in any
> way---bugfixes on our side have been reported to the upstream and we
> will import useful changes from there if any.  Although the current
> dot handling is useless, keeping the difference between the two small
> still has a meaning.

I agree.  Never fix vendor code.  Especially style bugs in it.  Not all
vendor code is in contrib.

Here is another comment nonsensified by removing its code:

% Author: eadler
% Date: Fri Jan 20 01:39:16 2012
% New Revision: 230360
% URL: http://svn.freebsd.org/changeset/base/230360
% 
% Log:
%   Fix warning when compiling with gcc46:
%   	error: variable 'flags' set but not used
% 
%   Approved by:	dim, cperciva (mentor, blanket for pre-mentorship already-approved commits)
%   MFC after:	3 days
% 
% Modified:
%   head/usr.sbin/cpucontrol/via.c
% 
% Modified: head/usr.sbin/cpucontrol/via.c
% ==============================================================================
% --- head/usr.sbin/cpucontrol/via.c	Fri Jan 20 01:39:08 2012	(r230359)
% +++ head/usr.sbin/cpucontrol/via.c	Fri Jan 20 01:39:16 2012	(r230360)
% @@ -82,7 +82,7 @@ via_update(const char *dev, const char *
%  	unsigned int i;
%  	size_t payload_size;
%  	via_fw_header_t *fw_header;
% -	uint32_t signature, flags;
% +	uint32_t signature;
%  	int32_t revision;
%  	void *fw_data;
%  	size_t data_size, total_size;
% @@ -121,7 +121,6 @@ via_update(const char *dev, const char *
%  	/*
%  	 * MSR_IA32_PLATFORM_ID contains flag in BCD in bits 52-50.
%  	 */
% -	flags = 1 << ((msrargs.data >> 50) & 7);
%  	msrargs.msr = MSR_BIOS_SIGN;
%  	error = ioctl(devfd, CPUCTL_RDMSR, &msrargs);
%  	if (error < 0) {
%

`flag' in bits 52-50 is apparently important enough to start a new section
of code with a block comment for, but was not used.  It can't be important
enough to start a new section of code with a block comment for deleted code.
in the block.

In fact, it seems that there there was a very large amount of unused code,
and this commit only removes the tip of it.  There is lots more code to
initialize msrargs.data in the above, but now msrargs.data is unised too.
This is apparently too complicated for the compiler to see that it is
unused:

% 	cpuctl_msr_args_t msrargs = {
% 		.msr = MSR_IA32_PLATFORM_ID,
% 	};

I don't like the style of this:
- initialization in declaration
- fairly complicated initialization in declaration
- 3 lines when 1 would do.

These obfuscations make it harder to see that this code is not really
used (it seems to be used only to initialize other variables that are
not used).  It initializes the full struct, but only msr in it is used,
and for later uses, the other parts are left as garbage (which is good
for saving space and indicating their non-use to the reader).  Later
uses initialize msr in a statement.  All uses should initialize it like
that.

I don't like this API.  style(9) forbids using typedefs to obfuscate
structs.  Here we have to know that it is a struct just to initialize
it.  So the typedef negatively opaque.

% 	cpuctl_cpuid_args_t idargs = {
% 		.level  = 1,	/* Signature. */
% 	};
% 	cpuctl_update_args_t args;
% 	int error;
% 
% 	assert(path);
% 	assert(dev);
% 
% 	fd = -1;
% 	devfd = -1;
% 	fw_image = MAP_FAILED;
% 	devfd = open(dev, O_RDWR);
% 	if (devfd < 0) {
% 		WARN(0, "could not open %s for writing", dev);
% 		return;
% 	}
% 	error = ioctl(devfd, CPUCTL_CPUID, &idargs);
% 	if (error < 0) {
% 		WARN(0, "ioctl(%s)", dev);
% 		goto fail;
% 	}
% 	signature = idargs.data[0];
% 	error = ioctl(devfd, CPUCTL_RDMSR, &msrargs);

Here we use msrargs.msr to initialize the full struct to useful values.
We previously initialized it fully, but to not very useful values:
- we initialized its msr to MSR_IA32_PLATFORM_ID, to obfuscate the
   msr that we are asking for here
- we initialized the rest of it to 0, to obfuscate that the rest is
   not used

% 	if (error < 0) {
% 		WARN(0, "ioctl(%s)", dev);
% 		goto fail;
% 	}
% 
% 	/*
% 	 * MSR_IA32_PLATFORM_ID contains flag in BCD in bits 52-50.
% 	 */

Now we have the result of requesting the msr in msrargs.data.  We used
to have the useless use of that, of assigning it to the unused `flags'
variable.  Now we don't have even that.  We only have its ghost in the
comment.

% 	msrargs.msr = MSR_BIOS_SIGN;
% 	error = ioctl(devfd, CPUCTL_RDMSR, &msrargs);

Here we ask for another msr.  This overwrites msrargs.data, leaving no
possible subsequent use for the result of the first ioctl.

% 	if (error < 0) {
% 		WARN(0, "ioctl(%s)", dev);
% 		goto fail;
% 	}
% 	revision = msrargs.data >> 32; /* Revision in the high dword. */

Here the result of the second ioctl is actually used.

Another bug in this API is that its struct members don't have prefixes.
One has the common English and programming name `data' so it is especially
hard to grep for.

I don't understand this code well enough to fix it.  Fixing it requires
understanding whether the unused variable was unused because of another
bug.  Another bug seems likely here, since there is so much dead code.
Or maybe I'm just confused, and the dead code is actually undead.  It
is complicated enough for this to be unclear.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120121103348.Q1254>