On Thu, Feb 01, 2018 at 02:50:19PM -0600, Eric Sandeen wrote:
> On 2/1/18 2:37 PM, Marco A Benatto wrote:
> > Currently, if the user switch source and target parameters
> > position, xfs_mdrestore truncates the dumpfile before abort
> > the execution.
> >
> > This patch checks the target parameter and if XFS_MD_MAGIC is
> > found, it aborts execution and leave dump file intact.
>
> Yeah, I think this is a good idea. It's pretty unforgiving
> to get it wrong and destroy your metadump in the process.
>
> (though tbh I usually get a compressed dump, and
> # bzcat dumpfile.bz2 | xfs_mdrestore - target
> works quite well and is more foolproof)
>
> So, despite the fact that I already talked with you about
> this a bit online, I think my subconscious was working on
> it in the meantime. What you've got is generally ok, but
> I wonder if the goal is to detect switched arguments, if
> it might be simpler to first validate the given source, and
> stop if it's not a metadump file. We've already got that in
> place, and it'd just be a question of checking the header
> unconditionally, something like this, which just moves some
> of the existing stuff work of the conditional:
>
> xfs_metablock_t mb;
>
>
> /* validate that our source file is really a metadump */
> if (fread(&mb, sizeof(mb), 1, src_f) != 1)
> fatal("error reading from source file: %s\n", strerror(errno));
>
> if (be32_to_cpu(mb.mb_magic) != XFS_MD_MAGIC)
> fatal("specified source file is not a metadata dump\n");
>
> if (show_info) {
> if (mb.mb_info & XFS_METADUMP_INFO_FLAGS) {
> printf("%s: %sobfuscated, %s log, %s metadata blocks\n",
> argv[optind],
> mb.mb_info & XFS_METADUMP_OBFUSCATED ? "":"not ",
> mb.mb_info & XFS_METADUMP_DIRTYLOG ? "dirty":"clean",
> mb.mb_info & XFS_METADUMP_FULLBLOCKS ? "full":"zeroed");
> } else {
> printf("%s: no informational flags present\n",
> argv[optind]);
> }
>
> if (argc - optind == 1)
> exit(0);
> }
>
> /* Go back to the beginning for the restore function */
> fseek(src_f, 0L, SEEK_SET);
So now you got /me/ looking at mdrestore and wondering, if the metadump
source is a pipe (as in your example above), won't this seek fail? We
already read the metadump super from the stream, so we can't seek
backwards and read it again:
# cat foo.md | xfs_mdrestore -i - /dev/sda
-: not obfuscated, clean log, full metadata blocks
xfs_mdrestore: specified file is not a metadata dump
Not to mention xfs_mdrestore --help doesn't tell you there's an -i
option.
Also, we already /do/ check that the source actually has a metadump
header, the problem is that we open the destination O_TRUNC before we
read the header and so if the target was a file, *poof* it's empty.
I think we'll need to fix both of those problems. Maybe we solve it by
opening src_f, checking the magic, optionally printing the metadump
info, and then passing both src_f and the header into perform_restore?
Separate patch, though.
> It's not explicitly checking the target, but it would probably serve
> the same purpose with fewer lines of new code. Either is ok with me,
> really. Any thoughts?
>
> Thanks,
> -Eric
>
>
> >
> > Signed-off-by: Marco A Benatto <marco.antonio.780@xxxxxxxxx>
> > ---
> > mdrestore/xfs_mdrestore.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> > index c49c13a..b4bf7b4 100644
> > --- a/mdrestore/xfs_mdrestore.c
> > +++ b/mdrestore/xfs_mdrestore.c
> > @@ -205,7 +205,7 @@ main(
> > int argc,
> > char **argv)
> > {
> > - FILE *src_f;
> > + FILE *src_f, *dst_f;
> > int dst_fd;
> > int c;
> > int open_flags;
> > @@ -277,6 +277,7 @@ main(
> >
> > optind++;
> >
> > +
> > /* check and open target */
> > open_flags = O_RDWR;
> > is_target_file = 0;
> > @@ -285,6 +286,19 @@ main(
> > open_flags |= O_CREAT;
> > is_target_file = 1;
> > } else if (S_ISREG(statbuf.st_mode)) {
> > + xfs_metablock_t mb;
dst_f could be declared here, right?
> > +
> > + dst_f = fopen(argv[optind], "rb");
> > + if (dst_f == NULL)
> > + fatal("cannot open target\n");
> > +
> > + if (fread(&mb, sizeof(mb), 1, dst_f) == 1) {
> > + if (be32_to_cpu(mb.mb_magic) == XFS_MD_MAGIC)
> > + fatal("target file is a xfs_metadump. switched arguments?\n");
> > + }
if (fread(&mb, sizeof(mb), 1, dst_f) == 1 &&
mb.mb_magic == cpu_to_be32(XFS_MD_MAGIC))
fatal("...");
?
--D
> > +
> > + fclose(dst_f);
> > +
> > open_flags |= O_TRUNC;
> > is_target_file = 1;
> > } else {
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
