linux-kernel - Re: [PATCH 2/7] Initial implementation of the trec driver and include files

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 for Android: free password hash cracker in your pocket

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

Message-ID: <46016208.3040303@saville.com>
Date:	Wed, 21 Mar 2007 09:49:12 -0700
From:	Wink Saville <wink@...ille.com>
To:	linux-kernel@...r.kernel.org,
	Johannes Weiner <hannes-kernel@...urebad.de>
Subject: Re: [PATCH 2/7] Initial implementation of the trec driver and include
 files

Johannes Weiner wrote:
> Your patch has broken lines where there shouldn't be any.
>   
OK.

>> +	struct trec_buffer_struct *	pNext;
>> +	struct trec_struct *		pCur;
>> +	struct trec_struct *		pEnd;
>> Please don't use camel-case - in general.
>>     
Would p_next, p_cur and p_end be OK?
>   
>> +static int snprint_address(char *b, int bsize, unsigned long address)
>> +{
>> +#ifdef CONFIG_KALLSYMS
>> +	unsigned long offset = 0, symsize;
>>     
>> +#else
>> +	return snprintf(b, bsize, "0x%016lx", address);
>> +#endif
>> +}
>>     
>
> Would it make sense to #ifdef the whole function?
> Make it static int (*)(...) if CONFIG_KALLSYMS and otherwise just a
> static inline int (*)(...) { return 0; }
>   
Maybe, but I think just letting the compiler decide is better.
>   
>> [...]
>> +static int trec_device_init(void)
>> +{
>> +	int 		result;
>> +		DPK("trec_device_init: cdev_add failed\n");
>> +		goto done;
>>     
>
> If you jump to `done' here, unregister_chrdev_region is never called.
>
> You should also declare trec_device_init as __init and trex_device_exit
> as __exit.
>   
I'll fix this.
>   
>> --- /dev/null
>> +++ b/include/linux/trec.h
>> @@ -0,0 +1,34 @@
>> +/*
>> +#define TREC0()			trec_write(TREC_PC_ADDR, TREC_PID, 0, 0)
>>     

>> +
>> +#define ZREC0()			do { } while (0)
>>     
> Why not seperate them by an #ifndef? So you don't have to replace TREC?
> with ZREC? but rather change one #define knob.
>
> =Hannes
>
>   

The reason is to allow the user easily change individual TREC's from 
active to inactive by just
changing a single character. Eventually I envision adding runtime 
support for changing if a
particular set of TREC's are active or not, but this was simple.

Thanks for the feed back.


Wink

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