xfs
[Top] [All Lists]

Re: [PATCH 3/5] metadump: separate single block objects from multiblock

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/5] metadump: separate single block objects from multiblock objects
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 3 Feb 2014 07:09:07 -0800
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1390472635-17225-4-git-send-email-david@xxxxxxxxxxxxx>
References: <1390472635-17225-1-git-send-email-david@xxxxxxxxxxxxx> <1390472635-17225-4-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Jan 23, 2014 at 09:23:53PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> When trying to dump objects, we have to treat multi-block objects
> differently to single block objects. Separate out the code paths for
> single block vs multi-block objects so we can add a separate path
> for multi-block objects.

Looks good, but two minor style nitpicks below.

Reviewed-by: Christoph Hellwig <hch@xxxxxx>

>  static int
> +process_multi_fsb_objects(
> +     xfs_dfiloff_t   o,
> +     xfs_dfsbno_t    s,
> +     xfs_dfilblks_t  c,
> +     typnm_t         btype,
> +     xfs_dfiloff_t   last)
> +{
> +     if (btype != TYP_DIR2) {
> +             print_warning("bad type for multi-fsb object %d", btype);
> +             return -EINVAL;
> +     }
> +
> +     return process_single_fsb_objects(o, s, c, btype, last);

I'd prefer a switch with a default statement for the unknown type here,
as that leads to nicer extensibility.

> +             /* single filesystem block objects are trivial to handle */
> +             if (btype != TYP_DIR2 || mp->m_dirblkfsbs == 1) {
> +                     error = process_single_fsb_objects(o, s, c, btype, 
> last);
> +                     if (error)
>                               return 0;
> +                     continue;
>               }
> +
> +             /* multi-extent directory blocks */
> +             error = process_multi_fsb_objects(o, s, c, btype, last);
> +             if (error)
> +                     return 0;

An if / else would look a little more obvious here, not that it really
matters all that much.

<Prev in Thread] Current Thread [Next in Thread>