Refactor xfs_repair's process_dinode_int
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 4 lines · Chandan Talukdar
Hi Barry,
- In
process_misc_ino_types(),dino->di_core.di_sizeis being accessed
without being converted to machine format. The check is being performed against 0; so, it should be fine. But for better code readability, I guess it should be accessed throughbe64_to_cpu().
Yeah... sort of in two-minds about this one.
Quoted 2 lines
- In
change_dinode_fmt(), it might be worthwhile to add an ASSERT against someone passing a value greater than 16 bit for 'new_fmt'.
Good idea.
Quoted 5 lines
- In
process_inode_attr_fork(),di_anextentsshould be accessed usingbe16_to_cpuas it is a 16 bit quantity.- In
process_dinode_int()line 2691,dinoc->di_extsizeshould be accessed usingbe32_to_cpu().
Good pickup on these, thanks :)
Quoted 2 lines
- In
process_dinode_int(), we should be checking for 'dblkmap' not being
NULL before freeing it. There are a few error conditions which can cause the control to go to 'clear_bad_out' with dblkmap being NULL.
freeing a NULL is valid, from the man page:
free() frees the memory space pointed to by ptr, which must have been returned by a previous call to malloc(), calloc() or realloc(). Otherwise, or if free(ptr) has already been called before, undefined behaviour occurs.
Quoted 4 lines
Quoted 1 line
Quoted 1 line
If ptr is NULL, no operation is performed. <<<
Thanks,
Chandan
Quoted 8 lines · Barry Naujok
Quoted 4 lines · Chandan Talukdar
Hi Barry,
- In
process_dinode_int(), we should be checking for 'dblkmap' not
being NULL before freeing it. There are a few error conditions which can cause the control to go to 'clear_bad_out' with dblkmap being NULL.freeing a NULL is valid
My bad! Yes, blkmap_free() doesn't check for a NULL pointer before going through the structure. I'll fix this up.
Regards,
Barry.
Not that it is a big deal....but my 2 cents...
Quoted 9 lines · Barry Naujok
Quoted 4 lines · Chandan Talukdar
Hi Barry,
- In
process_misc_ino_types(),dino->di_core.di_sizeis being accessed
without being converted to machine format. The check is being performed against 0; so, it should be fine. But for better code readability, I guess it should be accessed throughbe64_to_cpu().Yeah... sort of in two-minds about this one.
Well, traditionally we would not be endian converting it. We don't endian convert things which are compared to zero or are only 1 byte. There are a bunch of examples in the kernel code (many Christoph has done) and we should be consistent IMHO.
(There is, of course, no point from a code point of view - I guess you might consider that you are letting people know that we need to endian convert this value in general and that if we change the code in the future it might be needed... but just say no.:)
--Tim
Quoted 13 lines · Timothy Shimmin
Not that it is a big deal....but my 2 cents...
Quoted 10 lines · Barry Naujok
Quoted 6 lines · Chandan Talukdar
Hi Barry,
- In
process_misc_ino_types(),dino->di_core.di_sizeis being accessed without being converted to machine format. The check is being performed against 0; so, it should be fine. But for better code readability, I guess it should be accessed throughbe64_to_cpu().
Quoted 8 lines
Quoted 2 lines
Yeah... sort of in two-minds about this one.
Well, traditionally we would not be endian converting it. We don't endian convert things which are compared to zero or are only 1 byte. There are a bunch of examples in the kernel code (many Christoph has done) and we should be consistent IMHO.
(There is, of course, no point from a code point of view -
Exactly.
As a result the kernel does not have endian types for single byte variables (ie. there's __be16, __be32, __be64 but not __be8), nor are there cpu_to_be8 or be8_to_cpu conversion functions. Hence the lack of them in the XFS code ;)
Quoted 3 lines
I guess you might consider that you are letting people know that we need to endian convert this value in general and that if we change the code in the future it might be needed...
Well, that should be obvious when changing the structure that has lots of __beX types in already....
Quoted 1 line
but just say no.:)
Agreed ;)
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 11 lines · David Chinner
As a result the kernel does not have endian types for single byte variables (ie. there's
__be16,__be32,__be64but not__be8), nor are therecpu_to_be8orbe8_to_cpuconversion functions. Hence the lack of them in the XFS code ;)Quoted 3 lines
I guess you might consider that you are letting people know that we need to endian convert this value in general and that if we change the code in the future it might be needed...
Well, that should be obvious when changing the structure that has lots of
__beXtypes in already....
Actually enabling sparse checking for endianess annotations is still on my todo list. I started this project a while ago but failed when the cgcc wrapper had some problems interacting with libtool. I'll see how to get that resolved.