[PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields

[Ben Myers]

Hey Eric,

On Thu, Nov 01, 2012 at 11:33:46AM -0500, Eric Sandeen wrote:
> As xlog_print_trans_inode() stands today, it will error
> out if more than one flag is set on f->ilf_fields:
> 
> 	xlog_print_trans_inode: illegal inode type
> 
> but this is a perfectly valid case, to have i.e. a data and
> an attr flag set.
> 
> Following is a pretty big reworking of the function to
> handle more than one field type set.

I'm trying to wrap my head around this one.  I have a few stupid questions.

> I've tested this by a simple test such as creating one
> file on an selinux box, so that data+attr is set, and
> logprinting; I've also tested by running logprint after
> subsequent xfstest runs (although we hit other bugs that
> way).
> 
> Signed-off-by: Eric Sandeen <sandeen at redhat.com>
> ---
> 
> V2: Fix subject, sigh.
> 
> diff --git a/logprint/log_misc.c b/logprint/log_misc.c
> index e42e108..be2426e 100644
> --- a/logprint/log_misc.c
> +++ b/logprint/log_misc.c
> @@ -657,97 +657,84 @@ xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops)
>  
>      /* does anything come next */
>      op_head = (xlog_op_header_t *)*ptr;
> -    switch (f->ilf_fields & XFS_ILOG_NONCORE) {
> -	case XFS_ILOG_DEXT: {
> -	    ASSERT(f->ilf_size == 3);
> -	    (*i)++;
> -	    xlog_print_op_header(op_head, *i, ptr);
> -	    printf(_("EXTENTS inode data\n"));
> -	    *ptr += be32_to_cpu(op_head->oh_len);
> -	    if (XLOG_SET(op_head->oh_flags, XLOG_CONTINUE_TRANS))  {
> -		return 1;
> -	    }
> -	    break;
> -	}
> -	case XFS_ILOG_DBROOT: {
> -	    ASSERT(f->ilf_size == 3);
> -	    (*i)++;
> -	    xlog_print_op_header(op_head, *i, ptr);
> -	    printf(_("BTREE inode data\n"));
> -	    *ptr += be32_to_cpu(op_head->oh_len);
> -	    if (XLOG_SET(op_head->oh_flags, XLOG_CONTINUE_TRANS))  {
> -		return 1;
> -	    }
> +
> +    if (f->ilf_fields & (XFS_ILOG_DEV | XFS_ILOG_UUID)) {
> +	switch (f->ilf_fields & (XFS_ILOG_DEV | XFS_ILOG_UUID)) {

Here you kept only XFS_ILOG_DEV and XFS_ILOG_UUID...

> +	case XFS_ILOG_DEV:
> +	    printf(_("DEV inode: no extra region\n"));
>  	    break;
> -	}
> -	case XFS_ILOG_DDATA: {
> -	    ASSERT(f->ilf_size == 3);
> -	    (*i)++;
> -	    xlog_print_op_header(op_head, *i, ptr);
> -	    printf(_("LOCAL inode data\n"));
> -	    if (mode == S_IFDIR) {
> -		xlog_print_dir_sf((xfs_dir_shortform_t*)*ptr, size);
> -	    }
> -	    *ptr += be32_to_cpu(op_head->oh_len);
> -	    if (XLOG_SET(op_head->oh_flags, XLOG_CONTINUE_TRANS)) {
> -		return 1;
> -	    }
> +	case XFS_ILOG_UUID:
> +	    printf(_("UUID inode: no extra region\n"));
>  	    break;
> +	case XFS_ILOG_DEXT:
> +	case XFS_ILOG_DBROOT:
> +	case XFS_ILOG_DDATA:

Do you need to test for these other flags here?

> +	default:
> +	    xlog_panic(_("xlog_print_trans_inode: illegal inode type 0x%x"),
> +			f->ilf_fields);
>  	}
> -	case XFS_ILOG_AEXT: {
> -	    ASSERT(f->ilf_size == 3);
> +    }
> +
> +    if (f->ilf_fields & (XFS_ILOG_DFORK | XFS_ILOG_AFORK)) {
> +	ASSERT(f->ilf_size <= 4);

Can you explain this ASSERT?  I saw only ilf_size == 3 in the old code.  Under
what circumstances can it be 4?  Maybe when multiple ilf_fields are set?

> +	ASSERT((f->ilf_size == 3) || (f->ilf_fields & XFS_ILOG_AFORK));

I also don't understand this ASSERT.  It seems like in the old code all of the
AFORK related cases had an ASSERT for ilf_size == 3.

> +	if (f->ilf_fields & XFS_ILOG_DFORK) {
>  	    (*i)++;
>  	    xlog_print_op_header(op_head, *i, ptr);
> -	    printf(_("EXTENTS inode attr\n"));
> -	    *ptr += be32_to_cpu(op_head->oh_len);
> -	    if (XLOG_SET(op_head->oh_flags, XLOG_CONTINUE_TRANS))  {
> -		return 1;
> +
> +	    switch (f->ilf_fields & XFS_ILOG_DFORK) {
> +	    case XFS_ILOG_DEXT:
> +		printf(_("EXTENTS inode data\n"));
> +		break;
> +	    case XFS_ILOG_DBROOT:
> +		printf(_("BTREE inode data\n"));
> +		break;
> +	    case XFS_ILOG_DDATA:
> +		printf(_("LOCAL inode data\n"));
> +		if (mode == S_IFDIR)
> +		    xlog_print_dir_sf((xfs_dir_shortform_t*)*ptr, size);
> +		break;
> +	    case XFS_ILOG_DEV:
> +	    case XFS_ILOG_UUID:

ILOG_DEV and ILOG_UUID aren't in ILOG_DFORK.  You needn't test for them, correct?

I think I understand, just want to make sure.

Thanks,
	Ben