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

[Donald Douwsma <ddouwsma@redhat.com>]

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