Re: [PATCH] xfs_restore: remove DMAPI support

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Feb 10, 2022 at 04:46:13PM -0600, Eric Sandeen wrote:
> On 2/3/22 11:45 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The last of the DMAPI stubs were removed from Linux 5.17, so drop this
> > functionality altogether.
> 
> Why is this better than letting it EINVAL/ENOTTY/ENOWHATEVER when the
> ioctl gets called?

5.17 removed the ioctl definitions, so xfsdump won't build anymore.

> Though I don't really care, so I will go ahead and
> review it. :)
> 
> At this point I suppose finally pulling in Anthony's
> 	xfsdump: remove BMV_IF_NO_DMAPI_READ flag
> would make sense as well.

Yes.

> 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  doc/xfsdump.html  |    1 -
> >  po/de.po          |    5 ---
> >  po/pl.po          |    5 ---
> >  restore/content.c |   99 +++--------------------------------------------------
> >  restore/tree.c    |   33 ------------------
> >  restore/tree.h    |    1 -
> >  6 files changed, 6 insertions(+), 138 deletions(-)
> > 
> 
> ...
> 
> > diff --git a/restore/content.c b/restore/content.c
> > index 6b22965..e9b0a07 100644
> > --- a/restore/content.c
> > +++ b/restore/content.c
> > @@ -477,9 +477,6 @@ struct pers {
> >  			/* how many pages following the header page are reserved
> >  			 * for the subtree descriptors
> >  			 */
> > -		bool_t restoredmpr;
> > -			/* restore DMAPI event settings
> > -			 */
> >  		bool_t restoreextattrpr;
> >  			/* restore extended attributes
> >  			 */
> > @@ -858,7 +855,6 @@ static void partial_reg(ix_t d_index, xfs_ino_t ino, off64_t fsize,
> >                          off64_t offset, off64_t sz);
> >  static bool_t partial_check (xfs_ino_t ino, off64_t fsize);
> >  static bool_t partial_check2 (partial_rest_t *isptr, off64_t fsize);
> > -static int do_fssetdm_by_handle(char *path, fsdmidata_t *fdmp);
> 
> with fsdmidata_t completely gone I think its typedef can go too?
MProbably.


> ...
> 
> > @@ -8796,19 +8748,6 @@ restore_extattr(drive_t *drivep,
> >  			}
> >  		} else if (isfilerestored && path[0] != '\0') {
> >  			setextattr(path, ahdrp);
> 
> Pretty sure there's a hunk in setextattr that could go too, right?
> 
> @@ -8840,20 +8779,16 @@ restore_dir_extattr_cb_cb(extattrhdr_t *ahdrp, void *ctxp)
>  static void
>  setextattr(char *path, extattrhdr_t *ahdrp)
>  {
> -       static  char dmiattr[] = "SGI_DMI_";
>         bool_t isrootpr = ahdrp->ah_flags & EXTATTRHDR_FLAGS_ROOT;
>         bool_t issecurepr = ahdrp->ah_flags & EXTATTRHDR_FLAGS_SECURE;
> -       bool_t isdmpr;
>         int attr_namespace;
>         int rval;
>  
> -       isdmpr = (isrootpr &&
> -                  !strncmp((char *)(&ahdrp[1]), dmiattr, sizeof(dmiattr)-1));
>  
>         /* If restoreextattrpr not set, then we are here because -D was
>          * specified. So return unless it looks like a root DMAPI attribute.
>          */
> -       if (!persp->a.restoreextattrpr && !isdmpr)
> +       if (!persp->a.restoreextattrpr)
>                 return;

Er... yes?  Looks right, but xfsdump is enough of a mess... :/

> 
> > -
> > -			if (persp->a.dstdirisxfspr && persp->a.restoredmpr) {
> > -				int flag = 0;
> > -				char *attrname = (char *)&ahdrp[1];
> > -				if (ahdrp->ah_flags & EXTATTRHDR_FLAGS_ROOT)
> > -					flag = ATTR_ROOT;
> > -				else if (ahdrp->ah_flags & EXTATTRHDR_FLAGS_SECURE)
> > -					flag = ATTR_SECURE;
> > -
> > -				HsmRestoreAttribute(flag,
> > -						     attrname,
> > -						     &strctxp->sc_hsmflags);
> 
> And with the only user of strctxp gone it's now an unused local var, I think.

I don't do words that lack  ^^^^^^^ vowels.

> Anyway....
> 
> I wonder if there's still more that could be ripped out:
> 
>         uint32_t        bs_dmevmask;    /* DMI event mask        4    6c */
>         uint16_t        bs_dmstate;     /* DMI state info        2    6e */
> 
> Those can't go, I guess, because they are part of the header in the on-disk format.
> 
> But why are we still fiddling with them? For that matter, why does hsmapi.c still
> exist at all?

It probably can go too.

> I have the sense that if we really want to remove all dmapi support there's further
> to go, but as with all things xfsdump, it scares me a bit ...

<nod>

--D

> -Eric
>