[Ocfs2-tools-devel] corrupt_extent5.patch of modification with Sunil

[Sunil Mushran]

1. Please don't do this:
+errcode_t create_tmp_directory(ocfs2_filesys *fs,
+                               char *dirname, uint64_t *blkno)
+{
+       errcode_t ret = ocfs2_new_inode(fs, blkno, S_IFDIR | 0755);
+
+       if (ret)
+               goto new_inode_fail;

Use the normal style.

+errcode_t create_tmp_directory(ocfs2_filesys *fs,
+                               char *dirname, uint64_t *blkno)
+{
+       errcode_t ret;
+
+       ret = ocfs2_new_inode(fs, blkno, S_IFDIR | 0755);
+       if (ret)
+               goto new_inode_fail;

2. Use OCFS2_MAX_FILENAME_LEN.
+       char parentdir[256];

3. In ocfs2_extend_allocation_specific() the for loop is unorthodox.
Not incorrect but makes it unnecessarily unreadable.
Instead of:
+               for(i = n_clusters - 1;; i--){
+                       if (i == 0)
+                               break;
Do:
+               for(i = n_clusters; i; --i){

The latter is lot more readable. Adjust "i" in the loop.

4. In the same function, try not to do computation in each loop.
Use a variable to make life easier. While this is not much of an issue
in a tool, it becomes when you do the same in the module. Better you
learn to start thinking in terms of cpu usage now. :)
 >> blkno + i * blk_per_cluster
Redo inorder to eliminate the multiply.

5. Try not to name two functions literally the same. For e.g.,
damage_ocfs2_extent_blocks() and damage_ocfs2_extent_block().
I understand the reasoning but it makes for poor readability.
How about, damage_one_extent_block and damage_many_extent_blocks.

6. Overall try to provide helpful error messages where it happens rather 
than
in consolidated places. This is because it is easier to pinpoint the 
error location.
While this is not a core requirement for fswreck, it is for any end user 
tool.
(We are doing so because we've had a hard time diagnozing errors encountered
at ct sites.)

tao.ma wrote:
> Here is the new patch.
> Sunil Mushran wrote:
>
>> 1. When submitting a newer patch, increment the number so that there
>> is no confusion. I have multiple "corrupt_extent.patch" in my mbox. :)
>> corrupt_extent4.patch would have been clearer.
>
> Sorry. This time I make the new name corrupt_extent5.patch.
>
>>
>> 2. Use mktemp(3) instead of /dev/urandom to generate a random filename.
>> Even if you were to use the latter, you will need to convert the binary
>> to ascii (hex, maybe).
>
> Have modified it to mktemp.
>
>>
>> 3. line 761:
>> +               n_clusters = 1;
>> Is this necessary?
>
> Copied it from ocfs2_extend_allocation. Since it is no use, remove it.
>
>>
>> 4. FSWRK_FATAL should be used with care. It should be reserved
>> for cases for which we don't expect to encounter, like failed malloc,
>> and thus do not want to spend time coding the handling of the error.
>> In create_tmp_directory() you are handling the error condition but also
>> calling FATAL. I would rather that you handle the error itself.
>> Also, if you need to use it, make sure the error is not ambiguous.
>> For e.g., in ocfs2_extend_allocation_specific(), if one were to 
>> encounter
>> the error, it would not be immediately clear whether it was due to a 
>> failed
>> alloc or a failed insert. BTW, failed disk allocs should be handled 
>> and not
>> thrown to FATAL.
>
> Remove many call of FSWRK_FATAL and add my own error handling.
>
>>
>> 5. extend_file_size() does not look correct. You read_cached_inode()
>> twice yet free once. Memory leak.
>
>  Add another free calling. Thx.
>
>>
>> 6. Reuse existing types. For e.g., OCFS2_FT_DIR denotes a dir. The type
>> in extend_file_size introduces a new one.
>>
>> 7. See ocfs2_expand_dir() wrt the same function above. As in, I don't 
>> mind
>> if extend_file() just works on regular files and ignores dirs.
>
> Sorry, I think I use the wrong word of "type". So rewrite the function 
> and changed it to name "flag" and add two macros for code's readability.
>
>>
>> 8. Coding style. Always have a line separating the variable 
>> declaration section
>> and the code.
>
> Recheck the whole patch and add them. Thx.
>
>>
>> 9. Don't add Authors line in the copyright. We now add the author in
>> CREDITS rather than in each individual file. Year should be 2006.
>
> Remove them.
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Ocfs2-tools-devel mailing list
> Ocfs2-tools-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-tools-devel
>