* [PATCH] xfs_mdrestore: Don't restore over a dump file
@ 2018-02-01 20:37 Marco A Benatto
2018-02-01 20:50 ` Eric Sandeen
2018-02-01 21:54 ` Bill O'Donnell
0 siblings, 2 replies; 6+ messages in thread
From: Marco A Benatto @ 2018-02-01 20:37 UTC (permalink / raw)
To: linux-xfs
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.
Signed-off-by: Marco A Benatto <marco.antonio.780@gmail.com>
---
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 = 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");
+ }
+
+ fclose(dst_f);
+
open_flags |= O_TRUNC;
is_target_file = 1;
} else {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_mdrestore: Don't restore over a dump file
2018-02-01 20:37 [PATCH] xfs_mdrestore: Don't restore over a dump file Marco A Benatto
@ 2018-02-01 20:50 ` Eric Sandeen
2018-02-01 22:28 ` Darrick J. Wong
2018-02-01 21:54 ` Bill O'Donnell
1 sibling, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2018-02-01 20:50 UTC (permalink / raw)
To: Marco A Benatto, linux-xfs
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);
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@gmail.com>
> ---
> 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 = 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");
> + }
> +
> + fclose(dst_f);
> +
> open_flags |= O_TRUNC;
> is_target_file = 1;
> } else {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_mdrestore: Don't restore over a dump file
2018-02-01 20:37 [PATCH] xfs_mdrestore: Don't restore over a dump file Marco A Benatto
2018-02-01 20:50 ` Eric Sandeen
@ 2018-02-01 21:54 ` Bill O'Donnell
1 sibling, 0 replies; 6+ messages in thread
From: Bill O'Donnell @ 2018-02-01 21:54 UTC (permalink / raw)
To: Marco A Benatto; +Cc: linux-xfs
On Thu, Feb 01, 2018 at 06:37:43PM -0200, 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.
>
> Signed-off-by: Marco A Benatto <marco.antonio.780@gmail.com>
a few nits...otherwise,
Reviewed-by: Bill O'Donnell <billodo@redhat.com>
> ---
> 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++;
>
> +
^^^ extra space.
> /* 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 = 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");
^^^ exceeded 80 columns...probably ok for stdout messages... shrug.
> + }
> +
> + fclose(dst_f);
> +
> open_flags |= O_TRUNC;
> is_target_file = 1;
> } else {
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_mdrestore: Don't restore over a dump file
2018-02-01 20:50 ` Eric Sandeen
@ 2018-02-01 22:28 ` Darrick J. Wong
2018-02-01 22:39 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2018-02-01 22:28 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Marco A Benatto, linux-xfs
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@gmail.com>
> > ---
> > 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_mdrestore: Don't restore over a dump file
2018-02-01 22:28 ` Darrick J. Wong
@ 2018-02-01 22:39 ` Eric Sandeen
2018-02-02 17:52 ` Marco Benatto
0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2018-02-01 22:39 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Marco A Benatto, linux-xfs
On 2/1/18 4:28 PM, Darrick J. Wong wrote:
> 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.
<there is no --help> ;)
Neato!
> 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.
hhmm yeah.
> 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.
right, and make perform_restore know that we're already past the first
header block, right. Simply moving the first fread out of perform_restore
would do the trick. Would need to pass the read metablock into the
restore function.
yeah that makes sense.
>> 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@gmail.com>
>>> ---
>>> 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("...");
*nod*
-Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_mdrestore: Don't restore over a dump file
2018-02-01 22:39 ` Eric Sandeen
@ 2018-02-02 17:52 ` Marco Benatto
0 siblings, 0 replies; 6+ messages in thread
From: Marco Benatto @ 2018-02-02 17:52 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Darrick J. Wong, linux-xfs
Thanks all for the feedback and nice catch regarding the rewind + stdin reading.
I'm currently working on a patch which should cover (following
Sandeen's suggestions) the items Darrick has brought.
I should send this soon.
Thanks,
On Thu, Feb 1, 2018 at 8:39 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> On 2/1/18 4:28 PM, Darrick J. Wong wrote:
>> 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.
>
> <there is no --help> ;)
>
> Neato!
>
>> 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.
>
> hhmm yeah.
>
>> 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.
>
> right, and make perform_restore know that we're already past the first
> header block, right. Simply moving the first fread out of perform_restore
> would do the trick. Would need to pass the read metablock into the
> restore function.
>
> yeah that makes sense.
>
>>> 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@gmail.com>
>>>> ---
>>>> 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("...");
>
> *nod*
>
> -Eric
--
Marco Antonio Benatto
Linux user ID: #506236
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-02 17:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-01 20:37 [PATCH] xfs_mdrestore: Don't restore over a dump file Marco A Benatto
2018-02-01 20:50 ` Eric Sandeen
2018-02-01 22:28 ` Darrick J. Wong
2018-02-01 22:39 ` Eric Sandeen
2018-02-02 17:52 ` Marco Benatto
2018-02-01 21:54 ` Bill O'Donnell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).