linux-kernel - Re: [PATCH V8 08/13] posix clocks: cleanup the CLOCK

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: <alpine.LFD.2.00.1101111224280.12146@localhost6.localdomain6>
Date:	Tue, 11 Jan 2011 13:57:23 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Richard Cochran <richardcochran@...il.com>
cc:	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
	netdev@...r.kernel.org, Alan Cox <alan@...rguk.ukuu.org.uk>,
	Arnd Bergmann <arnd@...db.de>,
	Christoph Lameter <cl@...ux.com>,
	David Miller <davem@...emloft.net>,
	John Stultz <johnstul@...ibm.com>,
	Krzysztof Halasa <khc@...waw.pl>,
	Peter Zijlstra <peterz@...radead.org>,
	Rodolfo Giometti <giometti@...ux.it>
Subject: Re: [PATCH V8 08/13] posix clocks: cleanup the CLOCK_DISPTACH
 macro

On Fri, 31 Dec 2010, Richard Cochran wrote:
> +static inline bool clock_is_posix_cpu(const clockid_t id)
> +{
> +	if ((id & CLOCKFD_MASK) == CLOCKFD)
> +		return false;
> +	else
> +		return true;
> +}
> +
> +static inline int dispatch_clock_getres(const clockid_t id, struct timespec *ts)
> +{
> +	if (id >= 0) {
> +		return posix_clocks[id].clock_getres ?
> +			posix_clocks[id].clock_getres(id, ts) : -EINVAL;
> +	}
> +
> +	if (clock_is_posix_cpu(id))
> +		return posix_cpu_clock_getres(id, ts);

I wonder whether we should be a bit more clever here and create an
extra entry for posix_cpu_timers in the posix_clocks array and do the
following:

/* These are local to posix-timer.c and not exposed to anything else */
#define POSIX_INV_CLOCK_ID	MAX_CLOCKS
#define POSIX_CPU_CLOCK_ID	(MAX_CLOCKS + 1)
#define NR_CLOCK_ENTRIES	(MAX_CLOCKS + 2)

static struct k_clock posix_clocks[NR_CLOCK_ENTRIES];

static __init int init_posix_timers(void)
{
	......
	/*
	 * We leave the POSIX_INV_CLOCK_ID entries zeroed out, so the 
	 * the dispatch code will return -EINVAL.
	 */

	init_posix_cpu_timer_entries();
}

static clockid_t clock_get_array_id(const clockid_t id)
{
	if (id >= 0)
	       return id < MAX_CLOCKS ? id : POSIX_INV_CLOCK_ID;

      	if (clock_is_posix_cpu(id))
		return POSIX_CPU_CLOCK_ID;

	return POSIX_INV_CLOCK_ID;
}

static inline int dispatch_clock_getres(const clockid_t id, struct timespec *ts)
{
	struct k_clock *clk = &posix_clocks[clock_get_array_id(id)];

	return clk->clock_getres ? clk->clock_getres(id, ts) : -EINVAL;
}

That reduces the code significantly and the MAX_CLOCKS check in
clock_get_array_id() replaces the invalid_clock() check in the
syscalls as well. It does not matter whether we check this before or
after copying stuff from user.

Adding your new stuff requires just another entry in the array, the
setup of the function pointers and the CLOCKFD check in
clock_get_array_id(). Everything else just falls in place.

> +
> +#define CLOCK_DISPATCH(clock, call, arglist) dispatch_##call arglist
> +

Can we get rid of this completely please ?

>  clock_nanosleep_restart(struct restart_block *restart_block)
>  {
> -	clockid_t which_clock = restart_block->arg0;
> -

How does that compile ?

>  	return CLOCK_DISPATCH(which_clock, nsleep_restart,
>  			      (restart_block));
>  }

Thanks,

	tglx
--
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/