Arnd Bergmann wrote:
On Thursday 27 July 2006 07:40, Nitin Gupta wrote:
+#define ChunkFree(chunk) \
+ test_bit(CHUNK_free, (unsigned long *)(&(chunk)->size))
Your code has lots of CamelCase or MIXED_case in it, don't do that,
even if you find other people doing the same.
The two styles that are generally accepted are ALL_CAPS and all_lower_case.
Done s/CHUNK_free/CHUNK_FREE and like. 'Chunk' operations are used at my
places where they are done on 'struct page' too which uses CamelCase.
So, it may look uglier to have ALL_CAPS for chunk ops. like:
if (TestPageLocked(page))
SET_CHUNK_LOCKED(page);
instead of...
if (TestPageLocked(page))
SetChunkLocked(page);
This is not safe. By handing the *_bit() interface an unsigned long
pointer to 'unsigned short size' you're risking corrupting the chunk
members that are near the size member. Make 'size' an unsigned long and
get rid of these casts.
I need to keep 'struct chunk' as small as possible. This 'test_bit' usage is
just super blind copying of page-flags.h :)
I think this should do (atomic guarantee not required):
#define ChunkFree(chunk) \
(!!(chunk->size & (1 << CHUNK_FREE)))
#define SetChunkFree(chunk) \
(chunk->size |= (1 << CHUNK_FREE))
#define ClearChunkFree(chunk) \
(chunk->size &= ~(1 << CHUNK_FREE))
Or use __set_bit()/__clear_bit() to get the non-atomic versions of these.
test_bit() is always atomic and fast.
Again __set_bit()/__clear_bit() take unsigned long while chunk->size is
unsigned short. Can it cause problem if I cast this chunk->size to
unsigned long and pass to them? I think they just require start address
to count bit from and hence shouldn't hurt.
+/* error codes */
+#define CC_ENOMEM 1
+/* compression algo actually increased size! */
+#define CC_EEXPAND 2
+/* someone accessed page when we
+ * were about to compress it! */
+#define CC_EUSED 3
Please don't introduce your own error code namespace. Typical practice
is to use -ERRNO to indicate errors, 0 for success, and +ve for
indicating work done. Using a convention like that will make your code
more digestible to other kernel maintainers.
I couldn't find equivalent error codes with meaning of CC_EEXPAND and CC_EUSED
so defined them here. s/CC_ENOMEM/ENOMEM
s/CC_EUSED/EBUSY/ ?
Looks fine. I'll get some replacement for CC_EEXPAND too and get rid of
these :)
+int anon_cc_started=0;
+unsigned long max_anon_cc_size=0, max_fs_backed_cc_size=0;
+unsigned long anon_cc_size=0, fs_backed_cc_size=0; /* current size */
You don't need = 0 here, they'll be put in the BSS and zeroed if you
don't define an initial value.
Ok...But how do you avoid compiler warnings? Though thats not an issue but
they look ugly when compiling :)
What warning do you get from this?
None. These are global variables, didn't see that carefully. I get
'variable can be used uninitialized' for local vars. (when they indeed
can be used uninitialized).
+int cc_writepage(struct page *page)
+{
+ int ret = -CC_ENOMEM;
+ //swp_entry_t entry;
+ int comp_size, algo_idx;
+ struct chunk_head *ch=0;
+ struct page *tmp_page=0, *new_page=0, **slot;
Set pointers to NULL. Have you run sparse against this code? See
Documentation/sparse.txt in the kernel tree.
Having trouble understanding what it is....Will read more on it.
Even better, don't initialize any local variables in the declaration.
Normally, you have an assignment before the first use anyway.
Almost every time, case is this:
some_func()
{
void *a, *b;
a = alloc()
if (!a) goto out:
b = alloc();
...
out:
if (a) free(a)
if (b) free(b)
...
}
Here you (correctly) get 'possible uninitialized usage for b' warning.
That's why I initialize those local variables.
One more thing about comments. The common style is to
use either
/* single line comment */
or
/*
* multi
* line
* comment
*/
but not
/* multi
* line
* comment */
For disabling code fragments, use
#if 0
dont_call();
#endif
instead of
/* dont_call(); */
/*
* Ok, then
* I'll do this :)
*/
For the initial code, it also helps to run it through 'indent -kr -i8',
in order to get indentation right.
Arnd <><
Regards,
Nitin
_______________________________________________
Devel mailing list
[email protected]
http://mailman.laptop.org/mailman/listinfo/devel
