[Ocfs2-tools-devel] [PATCH 1/4] defrag.ocfs2: Basic framework borrowed from fsck.ocfs2

[Goldwyn Rodrigues]

Hi Joel,

On Fri, Nov 12, 2010 at 6:49 PM, Joel Becker <Joel.Becker at oracle.com> wrote:
> On Tue, Sep 07, 2010 at 10:49:05PM -0500, Goldwyn Rodrigues wrote:
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn at suse.de>
>
> Hey Goldwyn,
>        Sorry it's taken so long to have a look at these patches.  I'm
> hoping we can get them ready for inclusion soon.
>
>> +static void print_help(void)
>> +{
>> +     fprintf(stderr, "Usage: %s [-hvV] device\n", whoami);
>> +     fprintf(stderr, "Options:\n");
>> +     fprintf(stderr, "-h\t\t Print this help\n");
>> +     fprintf(stderr, "-v\t\t Verbose output\n");
>> +     fprintf(stderr, "-V\t\t Print version information\n");
>> +}
>
>        We definitely want help with the -1 and -2 options.

Yes, I will put that in.

>
>> +static errcode_t open_and_check(struct defrag_state *dst, char *filename,
>> +                             int open_flags, uint64_t blkno,
>> +                             uint64_t blksize)
>> +{
>> +     errcode_t ret;
>> +
>> +     ret = ocfs2_open(filename, open_flags, blkno, blksize, &dst->dst_fs);
>> +     if (ret) {
>> +             com_err(whoami, ret, "while opening \"%s\"", filename);
>> +             goto out;
>> +     }
>> +
>> +     if (ret) {
>> +             printf("Unrecoverable errors in the super block."
>> +                             "Cannot continue.\n");
>> +             goto out;
>> +     }
>
>        I think this second if(ret) is meaningless, right?

Yes, this perhaps remained from the last review.

>
>> +     printf("  De-fragmenting OCFS2 filesystem in %s:\n", filename);
>
>                                                'on' instead of 'in'.
>
>        I also would love to see the progress code used, but maybe you
> have that in later patches?
>

No, I haven't touched that part as yet. I was thinking of getting the
basic defragmentation working to start with.

-- 
Goldwyn