linux-kernel - Re: [PATCH] hpet: factor timer allocate from open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20100322152103.86cfec45.akpm@linux-foundation.org>
Date:	Mon, 22 Mar 2010 15:21:03 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Magnus Lynch <maglyx@...il.com>
Cc:	Clemens Ladisch <clemens@...isch.de>,
	"Venkatesh Pallipadi (Venki)" <venkatesh.pallipadi@...el.com>,
	"Vojtech Pavlik" <vojtech@...e.cz>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"Paul Gortmaker" <paul.gortmaker@...driver.com>,
	"Suresh Siddha" <suresh.b.siddha@...el.com>,
	"Thomas Gleixner" <tglx@...utronix.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hpet: factor timer allocate from open

On Thu, 18 Mar 2010 21:06:58 -0700 (PDT)
Magnus Lynch <maglyx@...il.com> wrote:

> Andrew Morton wrote:
> > Please always retain and maintain the changelog with each version of a patch.
> >
> > Please resend this patch with a complete changelog.
> 
> OK, here's my description from the original posting:
> <<
> The current implementation of the /dev/hpet driver couples opening the
> device with allocating one of the (scarce) timers (aka comparators).
> This is a limitation in that the main counter may be valuable to
> applications seeking a high-resolution timer who have no use for the
> interrupt generating functionality of the comparators.
> 
> This patch alters the open semantics so that when the device is
> opened, no timer is allocated. Operations that depend on a timer being
> in context implicitly attempt allocating a timer, to maintain backward
> compatibility. There is also an IOCTL (HPET_ALLOC_TIMER _IO) added so
> that the allocation may be done explicitly. (I prefer the explicit
> open then allocate pattern but don't know how practical it would be to
> require all existing code to be changed.)

A stylistic nit:

> @@ -384,6 +408,10 @@ static int hpet_fasync(int fd, struct file *file, int on)
>  {
>  	struct hpet_dev *devp;
>  
> +	int r = hpet_alloc_timer(file);
> +	if (r < 0)
> +		return r;
> +
>  	devp = file->private_data;
>  
>  	if (fasync_helper(fd, file, on, &devp->hd_async_queue) >= 0)
>
> ...
>
> @@ -438,6 +469,10 @@ hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>  {
>  	struct hpet_dev *devp;
>  
> +	int r = hpet_alloc_timer(file);
> +	if (r < 0)
> +		return r;
> +
>  	devp = file->private_data;
>  	return hpet_ioctl_common(devp, cmd, arg, 0);
>  }

The above constructs make it harder for people to modify the code
later.  If they want to add a new local, where to put it?  If they want
to add more code, where to put it?  Plus there are risks that people
will accidentally turn the code into c99-style definitions.

One could do

 {
 	struct hpet_dev *devp;
	int r = hpet_alloc_timer(file);

	if (r < 0)
		return r;


but that's not terribly good either: it adds risk that someone will
later add a leak.

Better is the plain old simple approach:


 {
 	struct hpet_dev *devp;
	int r;

	r = hpet_alloc_timer(file);
	if (r < 0)
		return r;


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/