[PATCH v2 0/3] xfsrestore: fix inventory unpacking

[PATCH v2 0/3] xfsrestore: fix inventory unpacking

[Donald Douwsma]

When xfsrestore reads its inventory from tape it fails to convert the media
record on bigendian systems, if the online inventory is unavailable this results
in invalid data being writen to the online inventory and failure to restore
non-directory files.

The series fixes the converstion and related issues.

---
v2
- Seperate out cleanup and content.c changes, fix whitespace.
- Show a full reproducer in the first patch.

Donald Douwsma (3):
  xfsrestore: fix inventory unpacking
  xfsrestore: stobj_unpack_sessinfo cleanup
  xfsrestore: untangle inventory unpacking logic

 inventory/inv_stobj.c | 40 ++++++++++++++--------------------------
 restore/content.c     | 13 +++++--------
 2 files changed, 19 insertions(+), 34 deletions(-)

-- 
2.31.1

[PATCH 1/3] xfsrestore: fix inventory unpacking

[Donald Douwsma]

When xfsrestore reads the inventory from tape media it fails to convert
media file records from bigendian. If the xfsdump inventory is not
available xfsrestore will write this invalid record to the on-line
inventory.

[root@rhel8 ~]# xfsdump -L Test1 -M "" -f /dev/st0 /boot > /dev/null
[root@rhel8 ~]# xfsdump -L Test2 -M "" -f /dev/st0 /boot > /dev/null
[root@rhel8 ~]# rm -rf /var/lib/xfsdump/inventory/
[root@rhel8 ~]# mt -f /dev/nst0 asf 2
[root@rhel8 ~]# xfsrestore -f /dev/nst0 -L Test2 /tmp/test2
xfsrestore: using scsi tape (drive_scsitape) strategy
xfsrestore: version 3.1.8 (dump format 3.0) - type ^C for status and control
xfsrestore: searching media for dump
xfsrestore: preparing drive
xfsrestore: examining media file 3
xfsrestore: found dump matching specified label:
xfsrestore: hostname: rhel8
xfsrestore: mount point: /boot
xfsrestore: volume: /dev/sda1
xfsrestore: session time: Tue Sep 27 16:05:28 2022
xfsrestore: level: 0
xfsrestore: session label: "Test2"
xfsrestore: media label: ""
xfsrestore: file system id: 26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
xfsrestore: session id: 62402423-7ae0-49ed-8ecb-9e5bc7642b91
xfsrestore: media id: 47ba45ca-3417-4006-ab10-3dc6419b83e2
xfsrestore: incorporating on-media session inventory into online inventory
xfsrestore: /var/lib/xfsdump/inventory created
xfsrestore: using on-media session inventory
xfsrestore: searching media for directory dump
xfsrestore: rewinding
xfsrestore: examining media file 0
xfsrestore: inventory session uuid (62402423-7ae0-49ed-8ecb-9e5bc7642b91) does not match the media header's session uuid (1771d9e8-a1ba-4e87-a61e-f6be97e41b45)
xfsrestore: examining media file 1
xfsrestore: inventory session uuid (62402423-7ae0-49ed-8ecb-9e5bc7642b91) does not match the media header's session uuid (1771d9e8-a1ba-4e87-a61e-f6be97e41b45)
xfsrestore: examining media file 2
xfsrestore: reading directories
xfsrestore: 9 directories and 320 entries processed
xfsrestore: directory post-processing
xfsrestore: restore complete: 0 seconds elapsed
xfsrestore: Restore Summary:
xfsrestore:   stream 0 /dev/nst0 OK (success)
xfsrestore: Restore Status: SUCCESS
[root@rhel8 ~]# xfsdump -I
file system 0:
        fs id:          26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
        session 0:
                mount point:    rhel8:/boot
                device:         rhel8:/dev/sda1
                time:           Tue Sep 27 16:05:28 2022
                session label:  "Test2"
                session id:     62402423-7ae0-49ed-8ecb-9e5bc7642b91
                level:          0
                resumed:        NO
                subtree:        NO
                streams:        1
                stream 0:
                        pathname:       /dev/st0
                        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:       47ba45ca-3417-4006-ab10-3dc6419b83e2
xfsdump: Dump Status: SUCCESS
[root@rhel8 ~]#
[root@rhel8 ~]# ls /tmp/test2
efi  grub2  loader

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, or if there is only one dump session on tape, xfsrestore is
somewhat inconsistent in this regard. Subsequent restores will use the
invalid on-line inventory and fail to restore files.

Fix this by correctly unpacking the inventory data.

Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
---
 inventory/inv_stobj.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
index c20e71c..5075ee4 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;
 
@@ -1087,26 +1087,13 @@ stobj_unpack_sessinfo(
 
 	/* 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
-- 
2.31.1

[PATCH 2/3] xfsrestore: stobj_unpack_sessinfo cleanup

[Donald Douwsma]

stobj_unpack_sessinfo should be the reverse of stobj_pack_sessinfo, make
this clearer with respect to the session header and streams processing.

signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
---
 inventory/inv_stobj.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
index 5075ee4..521acff 100644
--- a/inventory/inv_stobj.c
+++ b/inventory/inv_stobj.c
@@ -1065,25 +1065,26 @@ stobj_unpack_sessinfo(
 		return BOOL_FALSE;
 	}
 
+	/* get the seshdr and then, the remainder of the session */
 	xlate_invt_seshdr((invt_seshdr_t *)p, (invt_seshdr_t *)tmpbuf, 1);
 	bcopy(tmpbuf, p, sizeof(invt_seshdr_t));
-
-	/* get the seshdr and then, the remainder of the session */
 	s->seshdr = (invt_seshdr_t *)p;
 	s->seshdr->sh_sess_off = -1;
 	p += sizeof(invt_seshdr_t);
 
-
 	xlate_invt_session((invt_session_t *)p, (invt_session_t *)tmpbuf, 1);
 	bcopy (tmpbuf, p, sizeof(invt_session_t));
 	s->ses = (invt_session_t *)p;
 	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;
-- 
2.31.1

[PATCH 3/3] xfsrestore: untangle inventory unpacking logic

[Donald Douwsma]

stobj_unpack_sessinfo returns bool_t, fix logic in pi_addfile so errors
can be properly reported.

signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
---
 restore/content.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/restore/content.c b/restore/content.c
index b3999f9..04b6f81 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

Re: [PATCH 1/3] xfsrestore: fix inventory unpacking

[Darrick J. Wong]

On Wed, Sep 28, 2022 at 03:53:05PM +1000, Donald Douwsma wrote:
> When xfsrestore reads the inventory from tape media it fails to convert
> media file records from bigendian. If the xfsdump inventory is not
> available xfsrestore will write this invalid record to the on-line
> inventory.
> 
> [root@rhel8 ~]# xfsdump -L Test1 -M "" -f /dev/st0 /boot > /dev/null
> [root@rhel8 ~]# xfsdump -L Test2 -M "" -f /dev/st0 /boot > /dev/null
> [root@rhel8 ~]# rm -rf /var/lib/xfsdump/inventory/
> [root@rhel8 ~]# mt -f /dev/nst0 asf 2
> [root@rhel8 ~]# xfsrestore -f /dev/nst0 -L Test2 /tmp/test2
> xfsrestore: using scsi tape (drive_scsitape) strategy
> xfsrestore: version 3.1.8 (dump format 3.0) - type ^C for status and control
> xfsrestore: searching media for dump
> xfsrestore: preparing drive
> xfsrestore: examining media file 3
> xfsrestore: found dump matching specified label:
> xfsrestore: hostname: rhel8
> xfsrestore: mount point: /boot
> xfsrestore: volume: /dev/sda1
> xfsrestore: session time: Tue Sep 27 16:05:28 2022
> xfsrestore: level: 0
> xfsrestore: session label: "Test2"
> xfsrestore: media label: ""
> xfsrestore: file system id: 26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
> xfsrestore: session id: 62402423-7ae0-49ed-8ecb-9e5bc7642b91
> xfsrestore: media id: 47ba45ca-3417-4006-ab10-3dc6419b83e2
> xfsrestore: incorporating on-media session inventory into online inventory
> xfsrestore: /var/lib/xfsdump/inventory created
> xfsrestore: using on-media session inventory
> xfsrestore: searching media for directory dump
> xfsrestore: rewinding
> xfsrestore: examining media file 0
> xfsrestore: inventory session uuid (62402423-7ae0-49ed-8ecb-9e5bc7642b91) does not match the media header's session uuid (1771d9e8-a1ba-4e87-a61e-f6be97e41b45)
> xfsrestore: examining media file 1
> xfsrestore: inventory session uuid (62402423-7ae0-49ed-8ecb-9e5bc7642b91) does not match the media header's session uuid (1771d9e8-a1ba-4e87-a61e-f6be97e41b45)
> xfsrestore: examining media file 2
> xfsrestore: reading directories
> xfsrestore: 9 directories and 320 entries processed
> xfsrestore: directory post-processing
> xfsrestore: restore complete: 0 seconds elapsed
> xfsrestore: Restore Summary:
> xfsrestore:   stream 0 /dev/nst0 OK (success)
> xfsrestore: Restore Status: SUCCESS
> [root@rhel8 ~]# xfsdump -I
> file system 0:
>         fs id:          26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
>         session 0:
>                 mount point:    rhel8:/boot
>                 device:         rhel8:/dev/sda1
>                 time:           Tue Sep 27 16:05:28 2022
>                 session label:  "Test2"
>                 session id:     62402423-7ae0-49ed-8ecb-9e5bc7642b91
>                 level:          0
>                 resumed:        NO
>                 subtree:        NO
>                 streams:        1
>                 stream 0:
>                         pathname:       /dev/st0
>                         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:       47ba45ca-3417-4006-ab10-3dc6419b83e2
> xfsdump: Dump Status: SUCCESS
> [root@rhel8 ~]#
> [root@rhel8 ~]# ls /tmp/test2
> efi  grub2  loader
> 
> 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, or if there is only one dump session on tape, xfsrestore is
> somewhat inconsistent in this regard. Subsequent restores will use the
> invalid on-line inventory and fail to restore files.
> 
> Fix this by correctly unpacking the inventory data.
> 
> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
> ---
>  inventory/inv_stobj.c | 27 +++++++--------------------
>  1 file changed, 7 insertions(+), 20 deletions(-)
> 
> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
> index c20e71c..5075ee4 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;
>  
> @@ -1087,26 +1087,13 @@ stobj_unpack_sessinfo(
>  
>  	/* 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);

I wonder, do you need to preserve this behavior (writing the inventory
records to "moids")?  testmain.c seems to want to read the file, but
OTOH that looks like some sort of test program; arbitrarily overwriting
a file in $PWD seems ... risky?  And I guess this chunk has never run.

Also testmain.c has such gems as:

"/dana/hates/me"

"/the/krays"

and mentions that -I supersedes most of its functionality.  So maybe
that's why you deleted moids?  Aside from the name just sounding gross?

:)

> -		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, 

Nit: trailing whitespace                                      here ^

If the the answer to the above question is "Yeah, nobody wants the moids
file" then:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +					     (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
> -- 
> 2.31.1
> 

Re: [PATCH 2/3] xfsrestore: stobj_unpack_sessinfo cleanup

[Darrick J. Wong]

On Wed, Sep 28, 2022 at 03:53:06PM +1000, Donald Douwsma wrote:
> stobj_unpack_sessinfo should be the reverse of stobj_pack_sessinfo, make
> this clearer with respect to the session header and streams processing.
> 
> signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
> ---
>  inventory/inv_stobj.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
> index 5075ee4..521acff 100644
> --- a/inventory/inv_stobj.c
> +++ b/inventory/inv_stobj.c
> @@ -1065,25 +1065,26 @@ stobj_unpack_sessinfo(
>  		return BOOL_FALSE;
>  	}
>  
> +	/* get the seshdr and then, the remainder of the session */
>  	xlate_invt_seshdr((invt_seshdr_t *)p, (invt_seshdr_t *)tmpbuf, 1);
>  	bcopy(tmpbuf, p, sizeof(invt_seshdr_t));
> -
> -	/* get the seshdr and then, the remainder of the session */
>  	s->seshdr = (invt_seshdr_t *)p;
>  	s->seshdr->sh_sess_off = -1;
>  	p += sizeof(invt_seshdr_t);
>  
> -
>  	xlate_invt_session((invt_session_t *)p, (invt_session_t *)tmpbuf, 1);
>  	bcopy (tmpbuf, p, sizeof(invt_session_t));
>  	s->ses = (invt_session_t *)p;
>  	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, 

Nit: trailing whitespace                        here ^

> +				  (invt_stream_t *)tmpbuf, 1);
> +		bcopy(tmpbuf, p, sizeof(invt_stream_t));
> +		p += sizeof(invt_stream_t);

Ok, so we translate p into tmpbuf, then bcopy tmpbuf back to p.  That
part makes sense, but I am puzzled by what stobj_pack_sessinfo does:

	for (i = 0; i < ses->s_cur_nstreams; i++) {
		xlate_invt_stream(strms, (invt_stream_t *)sesbuf, 1);
		sesbuf += sizeof(invt_stream_t);
	}

Why isn't that callsite xlate_invt_stream(&strms[i], ...); ?

--D

> +	}
>  
>  	/* all the media files */
>  	s->mfiles = (invt_mediafile_t *)p;
> -- 
> 2.31.1
> 

Re: [PATCH 3/3] xfsrestore: untangle inventory unpacking logic

[Darrick J. Wong]

On Wed, Sep 28, 2022 at 03:53:07PM +1000, Donald Douwsma wrote:
> stobj_unpack_sessinfo returns bool_t, fix logic in pi_addfile so errors
> can be properly reported.
> 
> signed-off-by: Donald Douwsma <ddouwsma@redhat.com>

  ^ Needs correct capitalisation.

With that fixed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  restore/content.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/restore/content.c b/restore/content.c
> index b3999f9..04b6f81 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
> 

Re: [PATCH 1/3] xfsrestore: fix inventory unpacking

[Donald Douwsma]

On 29/09/2022 01:12, Darrick J. Wong wrote:
> On Wed, Sep 28, 2022 at 03:53:05PM +1000, Donald Douwsma wrote:
>> When xfsrestore reads the inventory from tape media it fails to convert
>> media file records from bigendian. If the xfsdump inventory is not
>> available xfsrestore will write this invalid record to the on-line
>> inventory.
>>
>> [root@rhel8 ~]# xfsdump -L Test1 -M "" -f /dev/st0 /boot > /dev/null
>> [root@rhel8 ~]# xfsdump -L Test2 -M "" -f /dev/st0 /boot > /dev/null
>> [root@rhel8 ~]# rm -rf /var/lib/xfsdump/inventory/
>> [root@rhel8 ~]# mt -f /dev/nst0 asf 2
>> [root@rhel8 ~]# xfsrestore -f /dev/nst0 -L Test2 /tmp/test2
>> xfsrestore: using scsi tape (drive_scsitape) strategy
>> xfsrestore: version 3.1.8 (dump format 3.0) - type ^C for status and control
>> xfsrestore: searching media for dump
>> xfsrestore: preparing drive
>> xfsrestore: examining media file 3
>> xfsrestore: found dump matching specified label:
>> xfsrestore: hostname: rhel8
>> xfsrestore: mount point: /boot
>> xfsrestore: volume: /dev/sda1
>> xfsrestore: session time: Tue Sep 27 16:05:28 2022
>> xfsrestore: level: 0
>> xfsrestore: session label: "Test2"
>> xfsrestore: media label: ""
>> xfsrestore: file system id: 26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
>> xfsrestore: session id: 62402423-7ae0-49ed-8ecb-9e5bc7642b91
>> xfsrestore: media id: 47ba45ca-3417-4006-ab10-3dc6419b83e2
>> xfsrestore: incorporating on-media session inventory into online inventory
>> xfsrestore: /var/lib/xfsdump/inventory created
>> xfsrestore: using on-media session inventory
>> xfsrestore: searching media for directory dump
>> xfsrestore: rewinding
>> xfsrestore: examining media file 0
>> xfsrestore: inventory session uuid (62402423-7ae0-49ed-8ecb-9e5bc7642b91) does not match the media header's session uuid (1771d9e8-a1ba-4e87-a61e-f6be97e41b45)
>> xfsrestore: examining media file 1
>> xfsrestore: inventory session uuid (62402423-7ae0-49ed-8ecb-9e5bc7642b91) does not match the media header's session uuid (1771d9e8-a1ba-4e87-a61e-f6be97e41b45)
>> xfsrestore: examining media file 2
>> xfsrestore: reading directories
>> xfsrestore: 9 directories and 320 entries processed
>> xfsrestore: directory post-processing
>> xfsrestore: restore complete: 0 seconds elapsed
>> xfsrestore: Restore Summary:
>> xfsrestore:   stream 0 /dev/nst0 OK (success)
>> xfsrestore: Restore Status: SUCCESS
>> [root@rhel8 ~]# xfsdump -I
>> file system 0:
>>          fs id:          26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
>>          session 0:
>>                  mount point:    rhel8:/boot
>>                  device:         rhel8:/dev/sda1
>>                  time:           Tue Sep 27 16:05:28 2022
>>                  session label:  "Test2"
>>                  session id:     62402423-7ae0-49ed-8ecb-9e5bc7642b91
>>                  level:          0
>>                  resumed:        NO
>>                  subtree:        NO
>>                  streams:        1
>>                  stream 0:
>>                          pathname:       /dev/st0
>>                          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:       47ba45ca-3417-4006-ab10-3dc6419b83e2
>> xfsdump: Dump Status: SUCCESS
>> [root@rhel8 ~]#
>> [root@rhel8 ~]# ls /tmp/test2
>> efi  grub2  loader
>>
>> 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, or if there is only one dump session on tape, xfsrestore is
>> somewhat inconsistent in this regard. Subsequent restores will use the
>> invalid on-line inventory and fail to restore files.
>>
>> Fix this by correctly unpacking the inventory data.
>>
>> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
>> ---
>>   inventory/inv_stobj.c | 27 +++++++--------------------
>>   1 file changed, 7 insertions(+), 20 deletions(-)
>>
>> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
>> index c20e71c..5075ee4 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;
>>   
>> @@ -1087,26 +1087,13 @@ stobj_unpack_sessinfo(
>>   
>>   	/* 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);
> 
> I wonder, do you need to preserve this behavior (writing the inventory
> records to "moids")?  testmain.c seems to want to read the file, but
> OTOH that looks like some sort of test program; arbitrarily overwriting
> a file in $PWD seems ... risky?  And I guess this chunk has never run.
> 
> Also testmain.c has such gems as:
> 
> "/dana/hates/me"
> 
> "/the/krays"
> 
> and mentions that -I supersedes most of its functionality.  So maybe
> that's why you deleted moids?  Aside from the name just sounding gross?
> 
> :)
> 

I think they were trying to mirror what's done in stobj_pack_sessinfo(),
which takes a file handle as a parameter, possibly its meant to provide
locking while packing for the inventory. Calling put_invtrecord without
having first called get_invtrecord seems unbalanced, and the unneeded
file "mods" was just left around. Either way the target for
stobj_unpack_sessinfo is a buffer not a file.

AFIKT this was a work in progress by someone who never got to finish it.

>> -		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,
> 
> Nit: trailing whitespace                                      here ^

Urg, sorry I thought I'd fixed all the white-space problems.

> 
> If the the answer to the above question is "Yeah, nobody wants the moids
> file" then:

Yes, I think moids was a hack that shouldn't have been there.

> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
>> +					     (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
>> -- 
>> 2.31.1
>>
> 

Re: [PATCH 2/3] xfsrestore: stobj_unpack_sessinfo cleanup

[Donald Douwsma]

On 29/09/2022 01:23, Darrick J. Wong wrote:
> On Wed, Sep 28, 2022 at 03:53:06PM +1000, Donald Douwsma wrote:
>> stobj_unpack_sessinfo should be the reverse of stobj_pack_sessinfo, make
>> this clearer with respect to the session header and streams processing.
>>
>> signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
>> ---
>>   inventory/inv_stobj.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
>> index 5075ee4..521acff 100644
>> --- a/inventory/inv_stobj.c
>> +++ b/inventory/inv_stobj.c
>> @@ -1065,25 +1065,26 @@ stobj_unpack_sessinfo(
>>   		return BOOL_FALSE;
>>   	}
>>   
>> +	/* get the seshdr and then, the remainder of the session */
>>   	xlate_invt_seshdr((invt_seshdr_t *)p, (invt_seshdr_t *)tmpbuf, 1);
>>   	bcopy(tmpbuf, p, sizeof(invt_seshdr_t));
>> -
>> -	/* get the seshdr and then, the remainder of the session */
>>   	s->seshdr = (invt_seshdr_t *)p;
>>   	s->seshdr->sh_sess_off = -1;
>>   	p += sizeof(invt_seshdr_t);
>>   
>> -
>>   	xlate_invt_session((invt_session_t *)p, (invt_session_t *)tmpbuf, 1);
>>   	bcopy (tmpbuf, p, sizeof(invt_session_t));
>>   	s->ses = (invt_session_t *)p;
>>   	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,
> 
> Nit: trailing whitespace                        here ^

nod

> 
>> +				  (invt_stream_t *)tmpbuf, 1);
>> +		bcopy(tmpbuf, p, sizeof(invt_stream_t));
>> +		p += sizeof(invt_stream_t);
> 
> Ok, so we translate p into tmpbuf, then bcopy tmpbuf back to p.  That
> part makes sense, but I am puzzled by what stobj_pack_sessinfo does:
> 
> 	for (i = 0; i < ses->s_cur_nstreams; i++) {
> 		xlate_invt_stream(strms, (invt_stream_t *)sesbuf, 1);
> 		sesbuf += sizeof(invt_stream_t);
> 	}
> 
> Why isn't that callsite xlate_invt_stream(&strms[i], ...); ?

Thanks! Yes, that's wrong, like the existing code it only worked/works
because there's only ever one stream. From the manpage "The third level
is media stream (currently only one  stream is supported)". Will fix.

> 
> --D
> 
>> +	}
>>   
>>   	/* all the media files */
>>   	s->mfiles = (invt_mediafile_t *)p;
>> -- 
>> 2.31.1
>>
> 

Re: [PATCH 3/3] xfsrestore: untangle inventory unpacking logic

[Donald Douwsma]

On 29/09/2022 01:24, Darrick J. Wong wrote:
> On Wed, Sep 28, 2022 at 03:53:07PM +1000, Donald Douwsma wrote:
>> stobj_unpack_sessinfo returns bool_t, fix logic in pi_addfile so errors
>> can be properly reported.
>>
>> signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
> 
>    ^ Needs correct capitalisation.
> 
> With that fixed,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Will fix for all three patches.

Thanks again,

Don

> 
>> ---
>>   restore/content.c | 13 +++++--------
>>   1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/restore/content.c b/restore/content.c
>> index b3999f9..04b6f81 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
>>
> 

Re: [PATCH 2/3] xfsrestore: stobj_unpack_sessinfo cleanup

[Donald Douwsma]

On 29/09/2022 09:12, Donald Douwsma wrote:
> 
> 
> On 29/09/2022 01:23, Darrick J. Wong wrote:
>> On Wed, Sep 28, 2022 at 03:53:06PM +1000, Donald Douwsma wrote:
>>> stobj_unpack_sessinfo should be the reverse of stobj_pack_sessinfo, make
>>> this clearer with respect to the session header and streams processing.
>>>
>>> signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
>>> ---
>>>   inventory/inv_stobj.c | 13 +++++++------
>>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
>>> index 5075ee4..521acff 100644
>>> --- a/inventory/inv_stobj.c
>>> +++ b/inventory/inv_stobj.c
>>> @@ -1065,25 +1065,26 @@ stobj_unpack_sessinfo(
>>>           return BOOL_FALSE;
>>>       }
>>> +    /* get the seshdr and then, the remainder of the session */
>>>       xlate_invt_seshdr((invt_seshdr_t *)p, (invt_seshdr_t *)tmpbuf, 1);
>>>       bcopy(tmpbuf, p, sizeof(invt_seshdr_t));
>>> -
>>> -    /* get the seshdr and then, the remainder of the session */
>>>       s->seshdr = (invt_seshdr_t *)p;
>>>       s->seshdr->sh_sess_off = -1;
>>>       p += sizeof(invt_seshdr_t);
>>> -
>>>       xlate_invt_session((invt_session_t *)p, (invt_session_t 
>>> *)tmpbuf, 1);
>>>       bcopy (tmpbuf, p, sizeof(invt_session_t));
>>>       s->ses = (invt_session_t *)p;
>>>       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,
>>
>> Nit: trailing whitespace                        here ^
> 
> nod
> 
>>
>>> +                  (invt_stream_t *)tmpbuf, 1);
>>> +        bcopy(tmpbuf, p, sizeof(invt_stream_t));
>>> +        p += sizeof(invt_stream_t);
>>
>> Ok, so we translate p into tmpbuf, then bcopy tmpbuf back to p.  That
>> part makes sense, but I am puzzled by what stobj_pack_sessinfo does:
>>
>>     for (i = 0; i < ses->s_cur_nstreams; i++) {
>>         xlate_invt_stream(strms, (invt_stream_t *)sesbuf, 1);
>>         sesbuf += sizeof(invt_stream_t);
>>     }
>>
>> Why isn't that callsite xlate_invt_stream(&strms[i], ...); ?
> 
> Thanks! Yes, that's wrong, like the existing code it only worked/works
> because there's only ever one stream. From the manpage "The third level
> is media stream (currently only one  stream is supported)". Will fix.

Or should I just drop this clean-up? I think what I'm saying is right,
but its a clean-up for a feature that cant be used. I doubt anyone is
going to add multiple stream support now, whatever that was intended
for.


> 
>>
>> --D
>>
>>> +    }
>>>       /* all the media files */
>>>       s->mfiles = (invt_mediafile_t *)p;
>>> -- 
>>> 2.31.1
>>>
>>

Re: [PATCH 2/3] xfsrestore: stobj_unpack_sessinfo cleanup

[Darrick J. Wong]

On Thu, Sep 29, 2022 at 09:28:24AM +1000, Donald Douwsma wrote:
> 
> 
> On 29/09/2022 09:12, Donald Douwsma wrote:
> > 
> > 
> > On 29/09/2022 01:23, Darrick J. Wong wrote:
> > > On Wed, Sep 28, 2022 at 03:53:06PM +1000, Donald Douwsma wrote:
> > > > stobj_unpack_sessinfo should be the reverse of stobj_pack_sessinfo, make
> > > > this clearer with respect to the session header and streams processing.
> > > > 
> > > > signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
> > > > ---
> > > >   inventory/inv_stobj.c | 13 +++++++------
> > > >   1 file changed, 7 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
> > > > index 5075ee4..521acff 100644
> > > > --- a/inventory/inv_stobj.c
> > > > +++ b/inventory/inv_stobj.c
> > > > @@ -1065,25 +1065,26 @@ stobj_unpack_sessinfo(
> > > >           return BOOL_FALSE;
> > > >       }
> > > > +    /* get the seshdr and then, the remainder of the session */
> > > >       xlate_invt_seshdr((invt_seshdr_t *)p, (invt_seshdr_t *)tmpbuf, 1);
> > > >       bcopy(tmpbuf, p, sizeof(invt_seshdr_t));
> > > > -
> > > > -    /* get the seshdr and then, the remainder of the session */
> > > >       s->seshdr = (invt_seshdr_t *)p;
> > > >       s->seshdr->sh_sess_off = -1;
> > > >       p += sizeof(invt_seshdr_t);
> > > > -
> > > >       xlate_invt_session((invt_session_t *)p, (invt_session_t
> > > > *)tmpbuf, 1);
> > > >       bcopy (tmpbuf, p, sizeof(invt_session_t));
> > > >       s->ses = (invt_session_t *)p;
> > > >       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,
> > > 
> > > Nit: trailing whitespace                        here ^
> > 
> > nod
> > 
> > > 
> > > > +                  (invt_stream_t *)tmpbuf, 1);
> > > > +        bcopy(tmpbuf, p, sizeof(invt_stream_t));
> > > > +        p += sizeof(invt_stream_t);
> > > 
> > > Ok, so we translate p into tmpbuf, then bcopy tmpbuf back to p.  That
> > > part makes sense, but I am puzzled by what stobj_pack_sessinfo does:
> > > 
> > >     for (i = 0; i < ses->s_cur_nstreams; i++) {
> > >         xlate_invt_stream(strms, (invt_stream_t *)sesbuf, 1);
> > >         sesbuf += sizeof(invt_stream_t);
> > >     }
> > > 
> > > Why isn't that callsite xlate_invt_stream(&strms[i], ...); ?
> > 
> > Thanks! Yes, that's wrong, like the existing code it only worked/works
> > because there's only ever one stream. From the manpage "The third level
> > is media stream (currently only one  stream is supported)". Will fix.
> 
> Or should I just drop this clean-up? I think what I'm saying is right,
> but its a clean-up for a feature that cant be used. I doubt anyone is
> going to add multiple stream support now, whatever that was intended
> for.

I don't know.  On the one hand I could see an argument for at least
being able to restore multiple streams, but then the dump program has
been screwing that up for years and nobody noticed.  I can't tell from
the source code what shape the multistream support is in, so I guess you
have some research to do? <shrug>  I suppose you could see what happens
if you try to set up multiple streams, but I have no idea ... what that
means.

Sorry that's a nonanswer. :(

--D

> 
> > 
> > > 
> > > --D
> > > 
> > > > +    }
> > > >       /* all the media files */
> > > >       s->mfiles = (invt_mediafile_t *)p;
> > > > -- 
> > > > 2.31.1
> > > > 
> > > 
> 

Re: [PATCH 2/3] xfsrestore: stobj_unpack_sessinfo cleanup

[Donald Douwsma]

On 30/09/2022 04:01, Darrick J. Wong wrote:
> On Thu, Sep 29, 2022 at 09:28:24AM +1000, Donald Douwsma wrote:
>>
>>
>> On 29/09/2022 09:12, Donald Douwsma wrote:
>>>
>>>
>>> On 29/09/2022 01:23, Darrick J. Wong wrote:
>>>> On Wed, Sep 28, 2022 at 03:53:06PM +1000, Donald Douwsma wrote:
>>>>> stobj_unpack_sessinfo should be the reverse of stobj_pack_sessinfo, make
>>>>> this clearer with respect to the session header and streams processing.
>>>>>
>>>>> signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
>>>>> ---
>>>>>    inventory/inv_stobj.c | 13 +++++++------
>>>>>    1 file changed, 7 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
>>>>> index 5075ee4..521acff 100644
>>>>> --- a/inventory/inv_stobj.c
>>>>> +++ b/inventory/inv_stobj.c
>>>>> @@ -1065,25 +1065,26 @@ stobj_unpack_sessinfo(
>>>>>            return BOOL_FALSE;
>>>>>        }
>>>>> +    /* get the seshdr and then, the remainder of the session */
>>>>>        xlate_invt_seshdr((invt_seshdr_t *)p, (invt_seshdr_t *)tmpbuf, 1);
>>>>>        bcopy(tmpbuf, p, sizeof(invt_seshdr_t));
>>>>> -
>>>>> -    /* get the seshdr and then, the remainder of the session */
>>>>>        s->seshdr = (invt_seshdr_t *)p;
>>>>>        s->seshdr->sh_sess_off = -1;
>>>>>        p += sizeof(invt_seshdr_t);
>>>>> -
>>>>>        xlate_invt_session((invt_session_t *)p, (invt_session_t
>>>>> *)tmpbuf, 1);
>>>>>        bcopy (tmpbuf, p, sizeof(invt_session_t));
>>>>>        s->ses = (invt_session_t *)p;
>>>>>        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,
>>>>
>>>> Nit: trailing whitespace                        here ^
>>>
>>> nod
>>>
>>>>
>>>>> +                  (invt_stream_t *)tmpbuf, 1);
>>>>> +        bcopy(tmpbuf, p, sizeof(invt_stream_t));
>>>>> +        p += sizeof(invt_stream_t);
>>>>
>>>> Ok, so we translate p into tmpbuf, then bcopy tmpbuf back to p.  That
>>>> part makes sense, but I am puzzled by what stobj_pack_sessinfo does:
>>>>
>>>>      for (i = 0; i < ses->s_cur_nstreams; i++) {
>>>>          xlate_invt_stream(strms, (invt_stream_t *)sesbuf, 1);
>>>>          sesbuf += sizeof(invt_stream_t);
>>>>      }
>>>>
>>>> Why isn't that callsite xlate_invt_stream(&strms[i], ...); ?
>>>
>>> Thanks! Yes, that's wrong, like the existing code it only worked/works
>>> because there's only ever one stream. From the manpage "The third level
>>> is media stream (currently only one  stream is supported)". Will fix.
>>
>> Or should I just drop this clean-up? I think what I'm saying is right,
>> but its a clean-up for a feature that cant be used. I doubt anyone is
>> going to add multiple stream support now, whatever that was intended
>> for.
> 
> I don't know.  On the one hand I could see an argument for at least
> being able to restore multiple streams, but then the dump program has
> been screwing that up for years and nobody noticed.  I can't tell from
> the source code what shape the multistream support is in, so I guess you
> have some research to do? <shrug>  I suppose you could see what happens
> if you try to set up multiple streams, but I have no idea ... what that
> means.
> 
> Sorry that's a nonanswer. :(

Actually this helped a lot, I realised that xfsdump takes multiple -f
arguments on the command line to setup the streams. This allowed me to
do some testing.

TLDR Using the current xfsprogs with multiple streams dumps ok (though
the second stream seems to end up with a higher end ino in its
inventory). But restore asserts when restoring the online inventory as
it fails to consume the media records [1].

With this series it restores ok and restores the inventory without
asserting [2].

If I drop this stobj_unpack_sessinfo cleanup patch we crash when
iterating over the media files for the streams[3].

I'll get a v3 series out including the change to stobj_pack_sessinfo()

Don


[1] Testing dump and restore with current xfsdump.

[root@rhel8 ~]# rpm -q xfsdump
xfsdump-3.1.8-2.el8.x86_64
[root@rhel8 ~]# xfsdump -L "Test1" -f /dev/nst0 -M tape1 -f /dev/nst1 -M 
tape2 /boot
xfsdump: using scsi tape (drive_scsitape) strategy
xfsdump: using scsi tape (drive_scsitape) strategy
xfsdump: version 3.1.8 (dump format 3.0) - type ^C for status and control
xfsdump: level 0 dump of rhel8:/boot
xfsdump: dump date: Thu Oct  6 13:50:45 2022
xfsdump: session id: aa25fa48-4493-45c7-9027-61e53e486445
xfsdump: session label: "Test1"
xfsdump: ino map phase 1: constructing initial dump list
xfsdump: ino map phase 2: skipping (no pruning necessary)
xfsdump: ino map phase 3: identifying stream starting points
xfsdump: stream 0: ino 133 offset 0 to ino 28839 offset 0
xfsdump: stream 1: ino 28839 offset 0 to end
xfsdump: ino map construction complete
xfsdump: estimated dump size: 328720704 bytes
xfsdump: estimated dump size per stream: 164375728 bytes
xfsdump: /var/lib/xfsdump/inventory created
xfsdump: drive 0: preparing drive
xfsdump: drive 1: preparing drive
xfsdump: drive 1: creating dump session media file 0 (media 0, file 0)
xfsdump: drive 1: dumping ino map
xfsdump: drive 1: dumping non-directory files
xfsdump: drive 0: creating dump session media file 0 (media 0, file 0)
xfsdump: drive 0: dumping ino map
xfsdump: drive 0: dumping directories
xfsdump: drive 0: dumping non-directory files
xfsdump: drive 1: ending media file
xfsdump: drive 1: media file size 166723584 bytes
xfsdump: drive 1: waiting for synchronized session inventory dump
xfsdump: drive 0: ending media file
xfsdump: drive 0: media file size 165675008 bytes
xfsdump: drive 0: waiting for synchronized session inventory dump
xfsdump: drive 0: dumping session inventory
xfsdump: drive 0: beginning inventory media file
xfsdump: drive 0: media file 1 (media 0, file 1)
xfsdump: drive 0: ending inventory media file
xfsdump: drive 0: inventory media file size 2097152 bytes
xfsdump: drive 0: writing stream terminator
xfsdump: drive 0: beginning media stream terminator
xfsdump: drive 0: media file 2 (media 0, file 2)
xfsdump: drive 0: ending media stream terminator
xfsdump: drive 0: media stream terminator size 1048576 bytes
xfsdump: drive 1: dumping session inventory
xfsdump: drive 1: beginning inventory media file
xfsdump: drive 1: media file 1 (media 0, file 1)
xfsdump: drive 1: ending inventory media file
xfsdump: drive 1: inventory media file size 2097152 bytes
xfsdump: drive 1: writing stream terminator
xfsdump: drive 1: beginning media stream terminator
xfsdump: drive 1: media file 2 (media 0, file 2)
xfsdump: drive 1: ending media stream terminator
xfsdump: drive 1: media stream terminator size 1048576 bytes
xfsdump: dump size (non-dir files) : 328189016 bytes
xfsdump: dump complete: 4 seconds elapsed
xfsdump: Dump Summary:
xfsdump:   stream 0 /dev/nst0 OK (success)
xfsdump:   stream 1 /dev/nst1 OK (success)
xfsdump: Dump Status: SUCCESS
[root@rhel8 ~]# xfsdump -I
file system 0:
	fs id:		26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
	session 0:
		mount point:	rhel8:/boot
		device:		rhel8:/dev/sda1
		time:		Thu Oct  6 13:50:45 2022
		session label:	"Test1"
		session id:	aa25fa48-4493-45c7-9027-61e53e486445
		level:		0
		resumed:	NO
		subtree:	NO
		streams:	2
		stream 0:
			pathname:	/dev/nst0
			start:		ino 133 offset 0
			end:		ino 28839 offset 0
			interrupted:	NO
			media files:	2
			media file 0:
				mfile index:	0
				mfile type:	data
				mfile size:	165675008
				mfile start:	ino 133 offset 0
				mfile end:	ino 28839 offset 0
				media label:	"tape1"
				media id:	adb31f2a-f026-4597-a20a-326f28ecbaf1
			media file 1:
				mfile index:	1
				mfile type:	inventory
				mfile size:	2097152
				media label:	"tape1"
				media id:	adb31f2a-f026-4597-a20a-326f28ecbaf1
		stream 1:
			pathname:	/dev/nst1
			start:		ino 28839 offset 0
			end:		ino 1572997 offset 0
			interrupted:	NO
			media files:	2
			media file 0:
				mfile index:	0
				mfile type:	data
				mfile size:	166723584
				mfile start:	ino 28839 offset 0
				mfile end:	ino 1572997 offset 0
				media label:	"tape2"
				media id:	22224f02-b6c7-47d5-ad61-a61ba071c8a8
			media file 1:
				mfile index:	1
				mfile type:	inventory
				mfile size:	2097152
				media label:	"tape2"
				media id:	22224f02-b6c7-47d5-ad61-a61ba071c8a8
xfsdump: Dump Status: SUCCESS
[root@rhel8 ~]# mv /var/lib/xfsdump/inventory 
/var/lib/xfsdump/inventory_two_sessions
[root@rhel8 ~]# xfsdump -I
xfsdump: Dump Status: SUCCESS

[root@rhel8 ~]# xfsrestore -L Test1 -f /dev/nst0 /tmp/test1/
xfsrestore: using scsi tape (drive_scsitape) strategy
xfsrestore: version 3.1.8 (dump format 3.0) - type ^C for status and control
xfsrestore: searching media for dump
xfsrestore: preparing drive
xfsrestore: examining media file 2
xfsrestore: found dump matching specified label:
xfsrestore: hostname: rhel8
xfsrestore: mount point: /boot
xfsrestore: volume: /dev/sda1
xfsrestore: session time: Thu Oct  6 13:50:45 2022
xfsrestore: level: 0
xfsrestore: session label: "Test1"
xfsrestore: media label: "tape1"
xfsrestore: file system id: 26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
xfsrestore: session id: aa25fa48-4493-45c7-9027-61e53e486445
xfsrestore: media id: adb31f2a-f026-4597-a20a-326f28ecbaf1
xfsrestore: searching media for directory dump
xfsrestore: rewinding
xfsrestore: examining media file 0
xfsrestore: reading directories
xfsrestore: 9 directories and 320 entries processed
xfsrestore: directory post-processing
xfsrestore: restoring non-directory files
xfsrestore: examining media file 1
xfsrestore: inv_stobj.c:1119: stobj_unpack_sessinfo: Assertion `(size_t) 
( p - (char *) bufp ) == bufsz' failed.
Aborted (core dumped)

1006 stobj_unpack_sessinfo(
...
1112         /* sanity check the size of the buffer given to us vs. the 
size it
1113            should be */
1114         if ((size_t) (p - (char *) bufp) != bufsz) {
1115                 mlog(MLOG_DEBUG | MLOG_INV, "p-bufp %d != bufsz %d 
entsz %d\n",
1116                       (int)(p - (char *) bufp), (int) bufsz,
1117               (int) (sizeof(invt_entry_t)));
1118         }
1119         assert((size_t) (p - (char *) bufp) == bufsz);


[2] Testing with this series

# remove the houskeeping dir
[root@rhel8 xfsdump-dev]# rm -rf /tmp/test1/*

[root@rhel8 xfsdump-dev]# restore/xfsrestore -L Test1 -f /dev/nst0 
/tmp/test1/
restore/xfsrestore: using scsi tape (drive_scsitape) strategy
restore/xfsrestore: version 3.1.10 (dump format 3.0) - type ^C for 
status and control
restore/xfsrestore: searching media for dump
restore/xfsrestore: preparing drive
restore/xfsrestore: examining media file 2
restore/xfsrestore: found dump matching specified label:
restore/xfsrestore: hostname: rhel8
restore/xfsrestore: mount point: /boot
restore/xfsrestore: volume: /dev/sda1
restore/xfsrestore: session time: Thu Oct  6 13:50:45 2022
restore/xfsrestore: level: 0
restore/xfsrestore: session label: "Test1"
restore/xfsrestore: media label: "tape1"
restore/xfsrestore: file system id: 26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
restore/xfsrestore: session id: aa25fa48-4493-45c7-9027-61e53e486445
restore/xfsrestore: media id: adb31f2a-f026-4597-a20a-326f28ecbaf1
restore/xfsrestore: searching media for directory dump
restore/xfsrestore: rewinding
restore/xfsrestore: examining media file 0
restore/xfsrestore: reading directories
restore/xfsrestore: 9 directories and 320 entries processed
restore/xfsrestore: directory post-processing
restore/xfsrestore: restoring non-directory files
restore/xfsrestore: examining media file 1
restore/xfsrestore: incorporating on-media session inventory into online 
inventory
restore/xfsrestore: /var/lib/xfsdump/inventory created
restore/xfsrestore: using on-media session inventory

  ============================ change media dialog 
=============================

please change media in drive
1: media change declined (timeout in 3600 sec)
2: display media inventory status
3: list needed media objects
4: media changed (default)
  -> 1
media change aborted

  --------------------------------- end dialog 
---------------------------------

restore/xfsrestore: NOTE: restore interrupted: 21 seconds elapsed: may 
resume later using -R option
restore/xfsrestore: Restore Summary:
restore/xfsrestore:   stream 0 /dev/nst0 QUIT (media is no longer usable)
restore/xfsrestore: Restore Status: QUIT
[root@rhel8 xfsdump-dev]#

[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:		Thu Oct  6 13:50:45 2022
		session label:	"Test1"
		session id:	aa25fa48-4493-45c7-9027-61e53e486445
		level:		0
		resumed:	NO
		subtree:	NO
		streams:	2
		stream 0:
			pathname:	/dev/nst0
			start:		ino 133 offset 0
			end:		ino 28839 offset 0
			interrupted:	YES
			media files:	1
			media file 0:
				mfile index:	0
				mfile type:	data
				mfile size:	165675008
				mfile start:	ino 133 offset 0
				mfile end:	ino 28839 offset 0
				media label:	"tape1"
				media id:	adb31f2a-f026-4597-a20a-326f28ecbaf1
		stream 1:
			pathname:	/dev/nst0
			start:		ino 133 offset 0
			end:		ino 28839 offset 0
			interrupted:	YES
			media files:	1
			media file 0:
				mfile index:	0
				mfile type:	data
				mfile size:	166723584
				mfile start:	ino 28839 offset 0
				mfile end:	ino 1572997 offset 0
				media label:	"tape2"
				media id:	22224f02-b6c7-47d5-ad61-a61ba071c8a8
xfsdump: Dump Status: SUCCESS
[root@rhel8 xfsdump-dev]#

[root@rhel8 xfsdump-dev]# mkdir /tmp/test1_
[root@rhel8 xfsdump-dev]# restore/xfsrestore -L Test1 -f /dev/nst0 -f 
/dev/nst1 /tmp/test1_
restore/xfsrestore: using scsi tape (drive_scsitape) strategy
restore/xfsrestore: using scsi tape (drive_scsitape) strategy
restore/xfsrestore: version 3.1.10 (dump format 3.0) - type ^C for 
status and control
restore/xfsrestore: drive 0: using online session inventory
restore/xfsrestore: drive 0: searching media for directory dump
restore/xfsrestore: drive 0: preparing drive
restore/xfsrestore: drive 0: examining media file 2
restore/xfsrestore: drive 0: rewinding
restore/xfsrestore: drive 0: examining media file 0
restore/xfsrestore: drive 0: reading directories
restore/xfsrestore: drive 0: 9 directories and 320 entries processed
restore/xfsrestore: drive 0: directory post-processing
restore/xfsrestore: drive 0: restoring non-directory files
restore/xfsrestore: drive 0: examining media file 1
restore/xfsrestore: drive 0: NOTE: please change media: type ^C to 
confirm media change
restore/xfsrestore: drive 1: preparing drive
restore/xfsrestore: drive 1: examining media file 2
restore/xfsrestore: drive 1: rewinding
restore/xfsrestore: drive 1: examining media file 0
restore/xfsrestore: drive 1: seeking past media file directory dump
restore/xfsrestore: drive 1: restoring non-directory files
restore/xfsrestore: drive 1: examining media file 1
restore/xfsrestore: restore complete: 3 seconds elapsed
restore/xfsrestore: Restore Summary:
restore/xfsrestore:   stream 0 /dev/nst0 ALREADY_DONE (another stream 
completed the operation)
restore/xfsrestore:   stream 1 /dev/nst1 OK (success)
restore/xfsrestore: Restore Status: SUCCESS
[root@rhel8 xfsdump-dev]# xfsdump -I


[3] Removing this stobj_unpack_sessinfo cleanup Segfaults.


[root@rhel8 xfsdump-dev]# gdb restore/xfsrestore
Reading symbols from restore/xfsrestore...done.
(gdb) run -L Test1 -f /dev/nst0 -f /dev/nst1 /tmp/test1
Starting program: 
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore -L Test1 -f 
/dev/nst0 -f /dev/nst1 /tmp/test1
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: using scsi tape 
(drive_scsitape) strategy
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: using scsi tape 
(drive_scsitape) strategy
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: version 3.1.10 
(dump format 3.0) - type ^C for status and control
[New Thread 0x7ffff6b5b700 (LWP 2251)]
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: 
searching media for dump
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: 
preparing drive
[New Thread 0x7ffff635a700 (LWP 2252)]
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 1: 
searching media for dump
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 1: 
preparing drive
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: 
examining media file 1
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: found 
dump matching specified label:
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: 
hostname: rhel8
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: mount 
point: /boot
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: 
volume: /dev/sda1
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: 
session time: Thu Oct  6 13:50:45 2022
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: level: 0
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: 
session label: "Test1"
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: media 
label: "tape1"
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: file 
system id: 26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: 
session id: aa25fa48-4493-45c7-9027-61e53e486445
/root/Devel/upstream/xfs/xfsdump-dev/restore/xfsrestore: drive 0: media 
id: adb31f2a-f026-4597-a20a-326f28ecbaf1

Thread 2 "xfsrestore" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff6b5b700 (LWP 2251)]
0x000000000041fce3 in stobj_unpack_sessinfo (bufp=<optimized out>, 
bufp@entry=0x7ffff7f42010, bufsz=<optimized out>, s=s@entry=0x7ffff6b59880)
     at inv_stobj.c:1094
1094				bcopy(tmpbuf, p, sizeof(invt_mediafile_t));
Missing separate debuginfos, use: yum debuginfo-install 
libuuid-2.32.1-27.el8.x86_64 xfsprogs-5.0.0-8.el8.x86_64
(gdb) bt
#0  0x000000000041fce3 in stobj_unpack_sessinfo (bufp=<optimized out>, 
bufp@entry=0x7ffff7f42010, bufsz=<optimized out>,
     s=s@entry=0x7ffff6b59880) at inv_stobj.c:1094
#1  0x000000000042525b in pi_addfile (mrhdrp=<optimized out>, 
scrhdrp=<optimized out>, drivep=0x65a700, drhdrp=<optimized out>,
     drhdrp=<optimized out>, grhdrp=<optimized out>, Mediap=<optimized 
out>) at content.c:5468
#2  0x000000000042c2ce in content_stream_restore (thrdix=thrdix@entry=0) 
at content.c:2198
#3  0x000000000041614e in childmain (arg1=0x0) at main.c:1465
#4  0x0000000000406daa in cldmgr_entry (arg1=0x652200 <cld>) at cldmgr.c:237
#5  0x00007ffff75a514a in start_thread (arg=<optimized out>) at 
pthread_create.c:479
#6  0x00007ffff72d4dc3 in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:95
(gdb) list
1089		s->mfiles = (invt_mediafile_t *)p;
1090		for (i=0; i< s->ses->s_cur_nstreams; i++) {
1091			for (j=0; j < s->strms[i].st_nmediafiles; j++) {
1092				xlate_invt_mediafile((invt_mediafile_t *)p,
1093						     (invt_mediafile_t *)tmpbuf, 1);
1094				bcopy(tmpbuf, p, sizeof(invt_mediafile_t));
1095				p +=  sizeof(invt_mediafile_t);
1096			}
1097		}
1098	
(gdb) info locals
i = <optimized out>
j = 832
tmpbuf = 0x7ffff0006f60 ""
p = 0x7ffff7f89f78 ""
__PRETTY_FUNCTION__ = "stobj_unpack_sessinfo"
__x = <optimized out>
__x = <optimized out>
(gdb)

We dont have 832 media files, i think this was setup to fail when didnt
decode the session's st_mediafiles. I steped over this in gdb and
confirmed that it was processing the second session when it did this.

(gdb) p *(s->strms+0)
$6 = {st_startino = {ino = 133, offset = 0}, st_endino = {ino = 28839, 
offset = 0}, st_firstmfile = 5408, st_lastmfile = 5760,
   st_cmdarg = "/dev/nst0", '\000' <repeats 246 times>, st_nmediafiles = 
2, st_interrupted = 1, st_padding = '\000' <repeats 15 times>}

(gdb) p *(s->strms+1)
$7 = {st_startino = {ino = 9583660007044415488, offset = 0}, st_endino = 
{ino = 12065143401725558784, offset = 0},
   st_firstmfile = 2311753983724617728, st_lastmfile = 
-9217179587367141376, st_cmdarg = "/dev/nst0", '\000' <repeats 246 times>,
   st_nmediafiles = 33554432, st_interrupted = 16777216, st_padding = 
'\000' <repeats 15 times>}
(gdb)

$ printf "0x%x\n" 33554432
0x2000000

As Darrick pointed out the second stream is a duplicate of the first
stream since stobj_pack_sessinfo just used the first stream for both
calls in the loop as confirmed by the duplicate st_cmdarg = "/dev/nst0.


> 
> --D
> 
>>
>>>
>>>>
>>>> --D
>>>>
>>>>> +    }
>>>>>        /* all the media files */
>>>>>        s->mfiles = (invt_mediafile_t *)p;
>>>>> -- 
>>>>> 2.31.1
>>>>>
>>>>
>>
>