* [PATCH] xfsrestore: fix inventory unpacking
@ 2022-09-14 3:47 Donald Douwsma
2022-09-14 17:30 ` Darrick J. Wong
0 siblings, 1 reply; 3+ messages in thread
From: Donald Douwsma @ 2022-09-14 3:47 UTC (permalink / raw)
To: linux-xfs; +Cc: Donald Douwsma
When xfsrestore reads the inventory from tape media it fails to convert
media file records from bigendin. If the xfsdump inventory is not
available xfsrestore will write this invalid record to the on-line
inventory.
[root@rhel8 xfsdump-dev]# xfsdump -I
file system 0:
fs id: 26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
session 0:
mount point: rhel8:/boot
device: rhel8:/dev/sda1
time: Fri Sep 9 14:29:03 2022
session label: ""
session id: 05f11cfe-2301-4000-89f2-2025091da413
level: 0
resumed: NO
subtree: NO
streams: 1
stream 0:
pathname: /dev/nst0
start: ino 133 offset 0
end: ino 1572997 offset 0
interrupted: YES
media files: 1
media file 0:
mfile index: 33554432
mfile type: data
mfile size: 211187836911616
mfile start: ino 9583660007044415488 offset 0
mfile end: ino 9583686395323482112 offset 0
media label: ""
media id: 4bf9ed40-6377-4926-be62-1bf7b59b1619
xfsdump: Dump Status: SUCCESS
The invalid start and end inode information cause xfsrestore to consider
that non-directory files do not reside in the current media and will
fail to restore them.
The behaviour of an initial restore may succeed if the position of the
tape is such that the data file is encountered before the inventory
file. Subsequent restores will use the invalid on-line inventory and
fail to restore files.
Fix this by correctly unpacking the inventory data. Also handle multiple
streams and untangle the logic where stobj_unpack_sessinfo is called.
Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
---
inventory/inv_stobj.c | 38 ++++++++++++++------------------------
restore/content.c | 13 +++++--------
2 files changed, 19 insertions(+), 32 deletions(-)
diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
index c20e71c..efaf46d 100644
--- a/inventory/inv_stobj.c
+++ b/inventory/inv_stobj.c
@@ -1008,7 +1008,7 @@ stobj_unpack_sessinfo(
size_t bufsz,
invt_sessinfo_t *s)
{
- uint i;
+ uint i, j;
char *tmpbuf;
char *p = (char *)bufp;
@@ -1080,35 +1080,25 @@ stobj_unpack_sessinfo(
p += sizeof(invt_session_t);
/* the array of all the streams belonging to this session */
- xlate_invt_stream((invt_stream_t *)p, (invt_stream_t *)tmpbuf, 1);
- bcopy(tmpbuf, p, sizeof(invt_stream_t));
s->strms = (invt_stream_t *)p;
- p += s->ses->s_cur_nstreams * sizeof(invt_stream_t);
+ for (i = 0; i < s->ses->s_cur_nstreams; i++) {
+ xlate_invt_stream((invt_stream_t *)p,
+ (invt_stream_t *)tmpbuf, 1);
+ bcopy(tmpbuf, p, sizeof(invt_stream_t));
+ p += sizeof(invt_stream_t);
+ }
/* all the media files */
s->mfiles = (invt_mediafile_t *)p;
-
-#ifdef INVT_DELETION
- {
- int tmpfd = open("moids", O_RDWR | O_CREAT, S_IRUSR|S_IWUSR);
- uint j;
- invt_mediafile_t *mmf = s->mfiles;
- for (i=0; i< s->ses->s_cur_nstreams; i++) {
- for (j=0; j< s->strms[i].st_nmediafiles;
- j++, mmf++)
- xlate_invt_mediafile((invt_mediafile_t *)mmf, (invt_mediafile_t *)tmpbuf, 1);
- bcopy(tmpbuf, mmf, sizeof(invt_mediafile_t));
- put_invtrecord(tmpfd, &mmf->mf_moid,
- sizeof(uuid_t), 0, SEEK_END, 0);
+ for (i=0; i< s->ses->s_cur_nstreams; i++) {
+ for (j=0; j < s->strms[i].st_nmediafiles; j++) {
+ xlate_invt_mediafile((invt_mediafile_t *)p,
+ (invt_mediafile_t *)tmpbuf, 1);
+ bcopy(tmpbuf, p, sizeof(invt_mediafile_t));
+ p += sizeof(invt_mediafile_t);
}
- close(tmpfd);
}
-#endif
- for (i = 0; i < s->ses->s_cur_nstreams; i++) {
- p += (size_t) (s->strms[i].st_nmediafiles)
- * sizeof(invt_mediafile_t);
- }
-
+
/* sanity check the size of the buffer given to us vs. the size it
should be */
if ((size_t) (p - (char *) bufp) != bufsz) {
diff --git a/restore/content.c b/restore/content.c
index b3999f9..bbced2d 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -5463,17 +5463,14 @@ pi_addfile(Media_t *Mediap,
* desc.
*/
sessp = 0;
- if (!buflen) {
- ok = BOOL_FALSE;
- } else {
- /* extract the session information from the buffer */
- if (stobj_unpack_sessinfo(bufp, buflen, &sessinfo)<0) {
- ok = BOOL_FALSE;
- } else {
+ ok = BOOL_FALSE;
+ /* extract the session information from the buffer */
+ if (buflen &&
+ stobj_unpack_sessinfo(bufp, buflen, &sessinfo)) {
stobj_convert_sessinfo(&sessp, &sessinfo);
ok = BOOL_TRUE;
- }
}
+
if (!ok || !sessp) {
mlog(MLOG_DEBUG | MLOG_WARNING | MLOG_MEDIA, _(
"on-media session "
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xfsrestore: fix inventory unpacking
2022-09-14 3:47 [PATCH] xfsrestore: fix inventory unpacking Donald Douwsma
@ 2022-09-14 17:30 ` Darrick J. Wong
2022-09-18 1:50 ` Donald Douwsma
0 siblings, 1 reply; 3+ messages in thread
From: Darrick J. Wong @ 2022-09-14 17:30 UTC (permalink / raw)
To: Donald Douwsma; +Cc: linux-xfs
On Wed, Sep 14, 2022 at 01:47:08PM +1000, Donald Douwsma wrote:
> When xfsrestore reads the inventory from tape media it fails to convert
> media file records from bigendin. If the xfsdump inventory is not
bigendian?
> available xfsrestore will write this invalid record to the on-line
> inventory.
>
> [root@rhel8 xfsdump-dev]# xfsdump -I
> file system 0:
> fs id: 26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
> session 0:
> mount point: rhel8:/boot
> device: rhel8:/dev/sda1
> time: Fri Sep 9 14:29:03 2022
> session label: ""
> session id: 05f11cfe-2301-4000-89f2-2025091da413
> level: 0
> resumed: NO
> subtree: NO
> streams: 1
> stream 0:
> pathname: /dev/nst0
> start: ino 133 offset 0
> end: ino 1572997 offset 0
> interrupted: YES
> media files: 1
> media file 0:
> mfile index: 33554432
> mfile type: data
> mfile size: 211187836911616
> mfile start: ino 9583660007044415488 offset 0
> mfile end: ino 9583686395323482112 offset 0
> media label: ""
> media id: 4bf9ed40-6377-4926-be62-1bf7b59b1619
> xfsdump: Dump Status: SUCCESS
>
> The invalid start and end inode information cause xfsrestore to consider
What's invalid here? I gather it's the transition from 1572997 to
9583660007044415488?
> that non-directory files do not reside in the current media and will
> fail to restore them.
>
> The behaviour of an initial restore may succeed if the position of the
> tape is such that the data file is encountered before the inventory
> file. Subsequent restores will use the invalid on-line inventory and
> fail to restore files.
>
> Fix this by correctly unpacking the inventory data.
Which chunk makes this happen? I'm afraid I'm not that familiar with
xfsrestore, so I can't really spot where the endian conversion is made.
Is it that chunk where you remove the "#ifdef INVT_DELETION" (which
AFAICT is never defined anywhere) and make it so that
xlate_invt_mediafile is always called?
> Also handle multiple
> streams and untangle the logic where stobj_unpack_sessinfo is called.
This sounds like multiple patches to me -- one to fix the missing
endianness conversion, another to handle the multiple streams, and a
third to do the cleanup in pi_addfile.
> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
> ---
> inventory/inv_stobj.c | 38 ++++++++++++++------------------------
> restore/content.c | 13 +++++--------
> 2 files changed, 19 insertions(+), 32 deletions(-)
>
> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
> index c20e71c..efaf46d 100644
> --- a/inventory/inv_stobj.c
> +++ b/inventory/inv_stobj.c
> @@ -1008,7 +1008,7 @@ stobj_unpack_sessinfo(
> size_t bufsz,
> invt_sessinfo_t *s)
> {
> - uint i;
> + uint i, j;
> char *tmpbuf;
> char *p = (char *)bufp;
>
> @@ -1080,35 +1080,25 @@ stobj_unpack_sessinfo(
> p += sizeof(invt_session_t);
>
> /* the array of all the streams belonging to this session */
> - xlate_invt_stream((invt_stream_t *)p, (invt_stream_t *)tmpbuf, 1);
> - bcopy(tmpbuf, p, sizeof(invt_stream_t));
> s->strms = (invt_stream_t *)p;
> - p += s->ses->s_cur_nstreams * sizeof(invt_stream_t);
> + for (i = 0; i < s->ses->s_cur_nstreams; i++) {
Indentation damage here.
> + xlate_invt_stream((invt_stream_t *)p,
> + (invt_stream_t *)tmpbuf, 1);
> + bcopy(tmpbuf, p, sizeof(invt_stream_t));
> + p += sizeof(invt_stream_t);
> + }
>
> /* all the media files */
> s->mfiles = (invt_mediafile_t *)p;
> -
> -#ifdef INVT_DELETION
> - {
> - int tmpfd = open("moids", O_RDWR | O_CREAT, S_IRUSR|S_IWUSR);
> - uint j;
> - invt_mediafile_t *mmf = s->mfiles;
> - for (i=0; i< s->ses->s_cur_nstreams; i++) {
> - for (j=0; j< s->strms[i].st_nmediafiles;
> - j++, mmf++)
> - xlate_invt_mediafile((invt_mediafile_t *)mmf, (invt_mediafile_t *)tmpbuf, 1);
> - bcopy(tmpbuf, mmf, sizeof(invt_mediafile_t));
> - put_invtrecord(tmpfd, &mmf->mf_moid,
> - sizeof(uuid_t), 0, SEEK_END, 0);
> + for (i=0; i< s->ses->s_cur_nstreams; i++) {
> + for (j=0; j < s->strms[i].st_nmediafiles; j++) {
> + xlate_invt_mediafile((invt_mediafile_t *)p,
> + (invt_mediafile_t *)tmpbuf, 1);
> + bcopy(tmpbuf, p, sizeof(invt_mediafile_t));
> + p += sizeof(invt_mediafile_t);
> }
> - close(tmpfd);
> }
> -#endif
> - for (i = 0; i < s->ses->s_cur_nstreams; i++) {
> - p += (size_t) (s->strms[i].st_nmediafiles)
> - * sizeof(invt_mediafile_t);
> - }
> -
> +
> /* sanity check the size of the buffer given to us vs. the size it
> should be */
> if ((size_t) (p - (char *) bufp) != bufsz) {
> diff --git a/restore/content.c b/restore/content.c
> index b3999f9..bbced2d 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -5463,17 +5463,14 @@ pi_addfile(Media_t *Mediap,
> * desc.
> */
> sessp = 0;
> - if (!buflen) {
> - ok = BOOL_FALSE;
> - } else {
> - /* extract the session information from the buffer */
> - if (stobj_unpack_sessinfo(bufp, buflen, &sessinfo)<0) {
> - ok = BOOL_FALSE;
> - } else {
> + ok = BOOL_FALSE;
> + /* extract the session information from the buffer */
> + if (buflen &&
There's a lot of trailing whitespace added by this patch.
Also, what is the purpose of this change? Is there something
significant about calling stobj_convert_sessinfo even if
stobj_unpack_sessinfo returns a negative number?
*OH* that unpack function returns "bool_t", which means that we only
care about zero and nonzero. So I think this is just a cleanup?
--D
> + stobj_unpack_sessinfo(bufp, buflen, &sessinfo)) {
> stobj_convert_sessinfo(&sessp, &sessinfo);
> ok = BOOL_TRUE;
> - }
> }
> +
> if (!ok || !sessp) {
> mlog(MLOG_DEBUG | MLOG_WARNING | MLOG_MEDIA, _(
> "on-media session "
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] xfsrestore: fix inventory unpacking
2022-09-14 17:30 ` Darrick J. Wong
@ 2022-09-18 1:50 ` Donald Douwsma
0 siblings, 0 replies; 3+ messages in thread
From: Donald Douwsma @ 2022-09-18 1:50 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On 15/09/2022 03:30, Darrick J. Wong wrote:
> On Wed, Sep 14, 2022 at 01:47:08PM +1000, Donald Douwsma wrote:
>> When xfsrestore reads the inventory from tape media it fails to convert
>> media file records from bigendin. If the xfsdump inventory is not
>
> bigendian?
oops, will fix.
>
>> available xfsrestore will write this invalid record to the on-line
>> inventory.
>>
>> [root@rhel8 xfsdump-dev]# xfsdump -I
>> file system 0:
>> fs id: 26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
>> session 0:
>> mount point: rhel8:/boot
>> device: rhel8:/dev/sda1
>> time: Fri Sep 9 14:29:03 2022
>> session label: ""
>> session id: 05f11cfe-2301-4000-89f2-2025091da413
>> level: 0
>> resumed: NO
>> subtree: NO
>> streams: 1
>> stream 0:
>> pathname: /dev/nst0
>> start: ino 133 offset 0
>> end: ino 1572997 offset 0
>> interrupted: YES
>> media files: 1
>> media file 0:
>> mfile index: 33554432
>> mfile type: data
>> mfile size: 211187836911616
>> mfile start: ino 9583660007044415488 offset 0
>> mfile end: ino 9583686395323482112 offset 0
>> media label: ""
>> media id: 4bf9ed40-6377-4926-be62-1bf7b59b1619
>> xfsdump: Dump Status: SUCCESS
>>
>> The invalid start and end inode information cause xfsrestore to consider
>
> What's invalid here? I gather it's the transition from 1572997 to
> 9583660007044415488?
Yes, for a single media file the inode information should be the same
as for the stream, for multiple media (big tape files that are split or
or multiple tapes) it should be the range of inodes contained in that
file. The index should have been 2 in this case, to match the location
of the file on tape.
>> that non-directory files do not reside in the current media and will
>> fail to restore them.
>>
>> The behaviour of an initial restore may succeed if the position of the
>> tape is such that the data file is encountered before the inventory
>> file. Subsequent restores will use the invalid on-line inventory and
>> fail to restore files.
>>
>> Fix this by correctly unpacking the inventory data.
>
> Which chunk makes this happen? I'm afraid I'm not that familiar with
> xfsrestore, so I can't really spot where the endian conversion is made.
> Is it that chunk where you remove the "#ifdef INVT_DELETION" (which
> AFAICT is never defined anywhere) and make it so that
> xlate_invt_mediafile is always called?
Yes, I think the code guarded by INVT_DELETION was a work in progress
during the initial port, that it describes this problem suggests they
knew when it would be needed. For Irix it would have worked as is, but
not for anything running on bigendian hardware.
>> Also handle multiple
>> streams and untangle the logic where stobj_unpack_sessinfo is called.
>
> This sounds like multiple patches to me -- one to fix the missing
> endianness conversion, another to handle the multiple streams, and a
> third to do the cleanup in pi_addfile.
>
Yes that should make this less confusing.
stobj_unpack_sessinfo() should be the reverse of stobj_pack_sessinfo()
but they don't read that way, I'll see if the session change makes more
seance as part of a clean-up to fix that.
Thanks Darrick!
Don
>> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
>> ---
>> inventory/inv_stobj.c | 38 ++++++++++++++------------------------
>> restore/content.c | 13 +++++--------
>> 2 files changed, 19 insertions(+), 32 deletions(-)
>>
>> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
>> index c20e71c..efaf46d 100644
>> --- a/inventory/inv_stobj.c
>> +++ b/inventory/inv_stobj.c
>> @@ -1008,7 +1008,7 @@ stobj_unpack_sessinfo(
>> size_t bufsz,
>> invt_sessinfo_t *s)
>> {
>> - uint i;
>> + uint i, j;
>> char *tmpbuf;
>> char *p = (char *)bufp;
>>
>> @@ -1080,35 +1080,25 @@ stobj_unpack_sessinfo(
>> p += sizeof(invt_session_t);
>>
>> /* the array of all the streams belonging to this session */
>> - xlate_invt_stream((invt_stream_t *)p, (invt_stream_t *)tmpbuf, 1);
>> - bcopy(tmpbuf, p, sizeof(invt_stream_t));
>> s->strms = (invt_stream_t *)p;
>> - p += s->ses->s_cur_nstreams * sizeof(invt_stream_t);
>> + for (i = 0; i < s->ses->s_cur_nstreams; i++) {
>
> Indentation damage here.
>
>> + xlate_invt_stream((invt_stream_t *)p,
>> + (invt_stream_t *)tmpbuf, 1);
>> + bcopy(tmpbuf, p, sizeof(invt_stream_t));
>> + p += sizeof(invt_stream_t);
>> + }
>>
>> /* all the media files */
>> s->mfiles = (invt_mediafile_t *)p;
>> -
>> -#ifdef INVT_DELETION
>> - {
>> - int tmpfd = open("moids", O_RDWR | O_CREAT, S_IRUSR|S_IWUSR);
>> - uint j;
>> - invt_mediafile_t *mmf = s->mfiles;
>> - for (i=0; i< s->ses->s_cur_nstreams; i++) {
>> - for (j=0; j< s->strms[i].st_nmediafiles;
>> - j++, mmf++)
>> - xlate_invt_mediafile((invt_mediafile_t *)mmf, (invt_mediafile_t *)tmpbuf, 1);
>> - bcopy(tmpbuf, mmf, sizeof(invt_mediafile_t));
>> - put_invtrecord(tmpfd, &mmf->mf_moid,
>> - sizeof(uuid_t), 0, SEEK_END, 0);
>> + for (i=0; i< s->ses->s_cur_nstreams; i++) {
>> + for (j=0; j < s->strms[i].st_nmediafiles; j++) {
>> + xlate_invt_mediafile((invt_mediafile_t *)p,
>> + (invt_mediafile_t *)tmpbuf, 1);
>> + bcopy(tmpbuf, p, sizeof(invt_mediafile_t));
>> + p += sizeof(invt_mediafile_t);
>> }
>> - close(tmpfd);
>> }
>> -#endif
>> - for (i = 0; i < s->ses->s_cur_nstreams; i++) {
>> - p += (size_t) (s->strms[i].st_nmediafiles)
>> - * sizeof(invt_mediafile_t);
>> - }
>> -
>> +
>> /* sanity check the size of the buffer given to us vs. the size it
>> should be */
>> if ((size_t) (p - (char *) bufp) != bufsz) {
>> diff --git a/restore/content.c b/restore/content.c
>> index b3999f9..bbced2d 100644
>> --- a/restore/content.c
>> +++ b/restore/content.c
>> @@ -5463,17 +5463,14 @@ pi_addfile(Media_t *Mediap,
>> * desc.
>> */
>> sessp = 0;
>> - if (!buflen) {
>> - ok = BOOL_FALSE;
>> - } else {
>> - /* extract the session information from the buffer */
>> - if (stobj_unpack_sessinfo(bufp, buflen, &sessinfo)<0) {
>> - ok = BOOL_FALSE;
>> - } else {
>> + ok = BOOL_FALSE;
>> + /* extract the session information from the buffer */
>> + if (buflen &&
>
> There's a lot of trailing whitespace added by this patch.
>
> Also, what is the purpose of this change? Is there something
> significant about calling stobj_convert_sessinfo even if
> stobj_unpack_sessinfo returns a negative number?
>
> *OH* that unpack function returns "bool_t", which means that we only
> care about zero and nonzero. So I think this is just a cleanup?
>
> --D
>
>> + stobj_unpack_sessinfo(bufp, buflen, &sessinfo)) {
>> stobj_convert_sessinfo(&sessp, &sessinfo);
>> ok = BOOL_TRUE;
>> - }
>> }
>> +
>> if (!ok || !sessp) {
>> mlog(MLOG_DEBUG | MLOG_WARNING | MLOG_MEDIA, _(
>> "on-media session "
>> --
>> 2.31.1
>>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-18 1:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-14 3:47 [PATCH] xfsrestore: fix inventory unpacking Donald Douwsma
2022-09-14 17:30 ` Darrick J. Wong
2022-09-18 1:50 ` Donald Douwsma
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.