[PATCH 0/9] Random xfsdump fixes

[PATCH 0/9] Random xfsdump fixes

[Eric Sandeen]

This is the result of needing some mindless work and going through
coverity errors.

It knocks out about 30 issues.

Tested w/ xfstests, -g dump.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/9] xfsdump: don't try to close fd from failed open

[Eric Sandeen]

I was going to just return if open() fails, but realized that
failing ioctls on the not-opened fd are ... maybe intentional?
By trying to run the ioctls on the invalid fd, it actually spits
useful error messages to the log, so...

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 restore/tree.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/restore/tree.c b/restore/tree.c
index 08e177f..fda4fc5 100644
--- a/restore/tree.c
+++ b/restore/tree.c
@@ -2626,7 +2626,8 @@ setdirattr( dah_t dah, char *path )
 		     path,
 		     strerror(errno));
 	}
-	( void )close( fd );
+	if (fd >= 0)
+		close( fd );
 }
 
 /* deletes orphanage if empty, else warns
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/9] xfsdump: Fix overflow of "question" string in Media_prompt_erase()

[Eric Sandeen]

The string we were sprintf'ing is trivially longer than the array
we allocated for it.

Allocate something big enough, and snprintf to avoid overflow

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 dump/content.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/dump/content.c b/dump/content.c
index ac19021..786fc52 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -6174,7 +6174,7 @@ static bool_t
 Media_prompt_overwrite( drive_t *drivep )
 {
 	fold_t fold;
-	char question[ 100 ];
+	char question[ 110 ];
 	char *preamblestr[ PREAMBLEMAX ];
 	size_t preamblecnt;
 	char *querystr[ QUERYMAX ];
@@ -6201,7 +6201,7 @@ retry:
 
 	/* query: ask if overwrite ok
 	 */
-	sprintf( question,
+	snprintf( question, sizeof(question),
 		 "overwrite data on media in "
 		 "drive %u?\n",
 		 (unsigned int)drivep->d_index );
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/9] xfsdump: fix wrong test for hard stack limit change failure

[Eric Sandeen]

We test if ( rlimit64.rlim_cur < minstacksz ) to see whether
we failed to increase rlimit64.rlim_max?  Weird.  Fix it.

(Granted, we raise both cur and max together, but it's odd to test
one, and report the other).

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 common/main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/common/main.c b/common/main.c
index 8e7451f..326f801 100644
--- a/common/main.c
+++ b/common/main.c
@@ -2077,7 +2077,7 @@ set_rlimits( size64_t *vmszp )
 			( void )setrlimit64( RLIMIT_STACK, &rlimit64 );
 			rval = getrlimit64( RLIMIT_STACK, &rlimit64 );
 			ASSERT( ! rval );
-			if ( rlimit64.rlim_cur < minstacksz ) {
+			if ( rlimit64.rlim_max < minstacksz ) {
 				mlog( MLOG_NORMAL
 				      |
 				      MLOG_WARNING
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/9] xfsdump: remove pointless if (dirty);

[Eric Sandeen]

"dirty" seems to be a remnant of unfinished code; other bits of
it are there but commented out, so comment out "if (dirty);" as
well, which is a no-op if statement which can't be true.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 inventory/inv_stobj.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
index becac17..84b15d9 100644
--- a/inventory/inv_stobj.c
+++ b/inventory/inv_stobj.c
@@ -976,7 +976,9 @@ stobj_delete_mobj(int fd,
 
 		}
 		free ( mfiles );
+/*
 		if ( dirty );
+*/
 	}
 
 	free ( strms );
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/9] xfsdump: remove some unreachable code

[Eric Sandeen]

in create_windows() we assign INFO_SIZE (which is defined to 4)
to the infosize variable, then test whether it is <= 0.  Remove
the pointless test.

in tsi_cmd_ls(), inside a if ( ! isdirpr ) block, we test whether
isdirpr might be true... remove it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 invutil/cmenu.c |    2 --
 restore/tree.c  |    5 ++---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/invutil/cmenu.c b/invutil/cmenu.c
index ff4abef..c4747b0 100644
--- a/invutil/cmenu.c
+++ b/invutil/cmenu.c
@@ -555,8 +555,6 @@ create_windows()
 	     menusize, COLS - 1);
 
     infosize = INFO_SIZE;
-    if(infosize <= 0)
-	infosize = 1;
 
     infowin = newwin(infosize, COLS, menusize + 1, 0);
     keypad(infowin, TRUE);
diff --git a/restore/tree.c b/restore/tree.c
index fda4fc5..21d3880 100644
--- a/restore/tree.c
+++ b/restore/tree.c
@@ -2905,11 +2905,10 @@ tsi_cmd_ls( void *ctxp,
 	 */
 	if ( ! isdirpr ) {
 		( * pcb )( pctxp,
-			   "    %s %10llu %s%s\n",
+			   "    %s %10llu %s\n",
 			   isselpr ? "*" : " ",
 			   ino,
-			   tranp->t_inter.i_name,
-			   isdirpr ? "/" : " " );
+			   tranp->t_inter.i_name);
 
 		return;
 	}
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 6/9] xfsdump: free suboptstrs if mlog_init1 fails

[Eric Sandeen]

suboptstrs was never freed on error paths.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 common/mlog.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/common/mlog.c b/common/mlog.c
index b0135b9..f1fa12f 100644
--- a/common/mlog.c
+++ b/common/mlog.c
@@ -149,6 +149,7 @@ mlog_init1( intgen_t argc, char *argv[ ] )
 	ix_t soix;
 	size_t vsymcnt;
 	intgen_t c;
+	bool_t retval = BOOL_FALSE;	/* Until proven otherwise */
 
 	/* prepare an array of suboption token strings. this will be the
 	 * concatenation of the subsystem names with the verbosity symbols.
@@ -192,7 +193,7 @@ mlog_init1( intgen_t argc, char *argv[ ] )
 					 progname,
 					 c );
 				usage( );
-				return BOOL_FALSE;
+				goto out_free;
 			}
 			options = optarg;
 			while ( *options ) {
@@ -208,7 +209,7 @@ mlog_init1( intgen_t argc, char *argv[ ] )
 						 progname,
 						 c );
 					usage( );
-					return BOOL_FALSE;
+					goto out_free;
 				}
 				ASSERT( ( ix_t )suboptix
 					<
@@ -224,7 +225,7 @@ mlog_init1( intgen_t argc, char *argv[ ] )
 							 c,
 						mlog_ss_names[ suboptix ] );
 						usage( );
-						return BOOL_FALSE;
+						goto out_free;
 					}
 					ssix = ( ix_t )suboptix;
 					mlog_level_ss[ ssix ] =
@@ -238,7 +239,7 @@ mlog_init1( intgen_t argc, char *argv[ ] )
 							 progname,
 							 c );
 						usage( );
-						return BOOL_FALSE;
+						goto out_free;
 					}
 					ssix = MLOG_SS_GEN;
 					mlog_level_ss[ ssix ] =
@@ -251,7 +252,7 @@ mlog_init1( intgen_t argc, char *argv[ ] )
 						 progname,
 						 c );
 					usage( );
-					return BOOL_FALSE;
+					goto out_free;
 				}
 			}
 			break;
@@ -267,8 +268,6 @@ mlog_init1( intgen_t argc, char *argv[ ] )
 		}
 	}
 
-	free( ( void * )suboptstrs );
-
 	/* give subsystems not explicitly called out the "general" verbosity
 	 */
 	for ( ssix = 0 ; ssix < MLOG_SS_CNT ; ssix++ ) {
@@ -301,7 +300,11 @@ mlog_init1( intgen_t argc, char *argv[ ] )
 
 	mlog_qlockh = QLOCKH_NULL;
 
-	return BOOL_TRUE;
+	retval = BOOL_TRUE;
+
+out_free:
+	free( ( void * )suboptstrs );
+	return retval;
 }
 
 bool_t
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 7/9] xfsdump: free allocated persistent paths

[Eric Sandeen]

"perspath" gets allocated by open_pathalloc, and haphazardly
freed at best; almost never on errors, often not on success.

While we're in there, fix a few leaked fd's as well.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 restore/content.c |    2 ++
 restore/inomap.c  |   11 +++++++++++
 restore/tree.c    |    9 +++++++++
 3 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/restore/content.c b/restore/content.c
index cfcf94d..c99aed7 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -1342,6 +1342,7 @@ content_init( intgen_t argc, char *argv[ ], size64_t vmsz )
 		      "could not open/create persistent state file %s: %s\n"),
 		      perspath,
 		      strerror( errno ));
+		free(perspath);
 		return BOOL_FALSE;
 	}
 
@@ -1356,6 +1357,7 @@ content_init( intgen_t argc, char *argv[ ], size64_t vmsz )
 		      "could not map persistent state file hdr %s: %s\n"),
 		      perspath,
 		      strerror( errno ));
+		free(perspath);
 		return BOOL_FALSE;
 	}
 
diff --git a/restore/inomap.c b/restore/inomap.c
index 562492e..1976017 100644
--- a/restore/inomap.c
+++ b/restore/inomap.c
@@ -217,6 +217,7 @@ inomap_restore_pers( drive_t *drivep,
 		      "could not open %s: %s\n"),
 		      perspath,
 		      strerror( errno ));
+		free(perspath);
 		return RV_ERROR;
 	}
 
@@ -233,6 +234,8 @@ inomap_restore_pers( drive_t *drivep,
 		      "unable to map %s: %s\n"),
 		      perspath,
 		      strerror( errno ));
+		close(fd);
+		free(perspath);
 		return RV_ERROR;
 	}
 
@@ -363,6 +366,7 @@ inomap_sync_pers( char *hkdir )
 	perspath = open_pathalloc( hkdir, PERS_NAME, 0 );
 	pers_fd = open( perspath, O_RDWR );
 	if ( pers_fd < 0 ) {
+		free(perspath);
 		return BOOL_TRUE;
 	}
 
@@ -377,6 +381,8 @@ inomap_sync_pers( char *hkdir )
 		      "unable to map %s hdr: %s\n"),
 		      perspath,
 		      strerror( errno ));
+		close(pers_fd);
+		free(perspath);
 		return BOOL_FALSE;
 	}
 
@@ -398,9 +404,14 @@ inomap_sync_pers( char *hkdir )
 		      "unable to map %s: %s\n"),
 		      perspath,
 		      strerror( errno ));
+		close(pers_fd);
+		free(perspath);
 		return BOOL_FALSE;
 	}
 
+	close(pers_fd);
+	free(perspath);
+
 	/* correct the next pointers
 	 */
 	for ( hnkp = roothnkp
diff --git a/restore/tree.c b/restore/tree.c
index 21d3880..011fa44 100644
--- a/restore/tree.c
+++ b/restore/tree.c
@@ -420,6 +420,7 @@ tree_init( char *hkdir,
 		      "could not open %s: %s\n"),
 		      perspath,
 		      strerror( errno ));
+		free(perspath);
 		return BOOL_FALSE;
 	}
 
@@ -435,6 +436,7 @@ tree_init( char *hkdir,
 		      "unable to map %s: %s\n"),
 		      perspath,
 		      strerror( errno ));
+		free(perspath);
 		return BOOL_FALSE;
 	}
 
@@ -443,9 +445,12 @@ tree_init( char *hkdir,
 	 */
 	ok = hash_init( vmsz / HASHSZ_PERVM, dircnt, nondircnt, perspath );
 	if ( ! ok ) {
+		free(perspath);
 		return BOOL_FALSE;
 	}
 
+	free(perspath);
+
 	/* initialize the node abstraction. let it's use of backing store
 	 * begin immediately after the hash abstraction. give it the remainder
 	 * of vm.
@@ -595,6 +600,7 @@ tree_sync( char *hkdir,
 		      "could not open %s: %s\n"),
 		      perspath,
 		      strerror( errno ));
+		free(perspath);
 		return BOOL_FALSE;
 	}
 
@@ -610,6 +616,7 @@ tree_sync( char *hkdir,
 		      "unable to map %s: %s\n"),
 		      perspath,
 		      strerror( errno ));
+		free(perspath);
 		return BOOL_FALSE;
 	}
 
@@ -632,9 +639,11 @@ tree_sync( char *hkdir,
 	 */
 	ok = hash_sync( perspath );
 	if ( ! ok ) {
+		free(perspath);
 		return BOOL_FALSE;
 	}
 
+	free(perspath);
 	/* synchronize with the node abstraction.
 	 */
 	ASSERT( persp->p_hashsz <= ( size64_t )( OFF64MAX - ( off64_t )PERSSZ));
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 8/9] xfsdump: children[] is an array of child pointers, not of child structures

[Eric Sandeen]

node_t      **children;     /* child nodes */

so allocating space to hold nbr_children structures is wrong;
we need to hold nbr_children _pointers_ to child structures.

It's over-allocating, so it's relatively harmless.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 invutil/list.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/invutil/list.c b/invutil/list.c
index 0157102..599e6af 100644
--- a/invutil/list.c
+++ b/invutil/list.c
@@ -34,7 +34,7 @@ parent_add_child(node_t *parent, node_t *child)
     d = parent->data;
 
     d->nbr_children++;
-    d->children = realloc(d->children, d->nbr_children * sizeof(*child));
+    d->children = realloc(d->children, d->nbr_children * sizeof(node_t *));
     d->children[d->nbr_children - 1] = child;
 
     return parent;
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 9/9] xfsdump: fix uninit ackstr in content_mediachange_query()

[Eric Sandeen]

Today, this sends an uninitialized ackstr[0] to be mlog'd -
who knows what we get out of it.  Other places follow this
"count = 0, string = "\n"" pattern which seemsa bit odd, but
better than printing uninitialized memory.

To be completely honest, I have no test for this.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 restore/content.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/restore/content.c b/restore/content.c
index c99aed7..bc5b398 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -2915,6 +2915,7 @@ content_mediachange_query( void )
 			bagp = 0;
 		}
 		ackcnt = 0;
+		ackstr[ ackcnt++ ] = "\n";
 		dlog_multi_ack( ackstr,
 				ackcnt );
 		querycnt = 0;
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/9] xfsdump: Fix overflow of "question" string in Media_prompt_erase()

[Christoph Hellwig]

On Wed, Nov 12, 2014 at 12:57:11PM -0600, Eric Sandeen wrote:
> The string we were sprintf'ing is trivially longer than the array
> we allocated for it.
> 
> Allocate something big enough, and snprintf to avoid overflow

How about switching it to asprintf() to get rid of that dangerous static
buffer entirely?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/9 V2] xfsdump: Fix overflow of "question" string in Media_prompt_erase()

[Eric Sandeen]

The string we were sprintf'ing is trivially longer than the array
we allocated for it.

Use asprintf instead, as suggested by Christoph.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: use asprintf

diff --git a/dump/content.c b/dump/content.c
index ac19021..43756e3 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -6174,7 +6174,7 @@ static bool_t
 Media_prompt_overwrite( drive_t *drivep )
 {
 	fold_t fold;
-	char question[ 100 ];
+	char *question;
 	char *preamblestr[ PREAMBLEMAX ];
 	size_t preamblecnt;
 	char *querystr[ QUERYMAX ];
@@ -6201,9 +6201,8 @@ retry:
 
 	/* query: ask if overwrite ok
 	 */
-	sprintf( question,
-		 "overwrite data on media in "
-		 "drive %u?\n",
+	asprintf( &question, 
+		 "overwrite data on media in drive %u?\n",
 		 (unsigned int)drivep->d_index );
 	querycnt = 0;
 	querystr[ querycnt++ ] = question;
@@ -6250,6 +6249,8 @@ retry:
 	dlog_end( postamblestr,
 		  postamblecnt );
 
+	free(question);
+
 	if ( responseix == sigintix ) {
 		if ( cldmgr_stop_requested( )) {
 			return BOOL_FALSE;


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/9] xfsdump: remove pointless if (dirty);

[Mark Tinguely]

On 11/12/14 12:57, Eric Sandeen wrote:
> "dirty" seems to be a remnant of unfinished code; other bits of
> it are there but commented out, so comment out "if (dirty);" as
> well, which is a no-op if statement which can't be true.
>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---
>   inventory/inv_stobj.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
> index becac17..84b15d9 100644
> --- a/inventory/inv_stobj.c
> +++ b/inventory/inv_stobj.c
> @@ -976,7 +976,9 @@ stobj_delete_mobj(int fd,
>
>   		}
>   		free ( mfiles );
> +/*
>   		if ( dirty );
> +*/
>   	}
>
>   	free ( strms );

Why not remove all the references to dirty in the function?
Like you said, it doesn't do anything.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/9 V2] xfsdump: Fix overflow of "question" string in Media_prompt_erase()

[Christoph Hellwig]

> +	asprintf( &question, 
> +		 "overwrite data on media in drive %u?\n",
>  		 (unsigned int)drivep->d_index );

Where is the error handling?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/9] xfsdump: remove pointless if (dirty);

[Eric Sandeen]

On 11/13/14 11:35 AM, Mark Tinguely wrote:
> On 11/12/14 12:57, Eric Sandeen wrote:
>> "dirty" seems to be a remnant of unfinished code; other bits of
>> it are there but commented out, so comment out "if (dirty);" as
>> well, which is a no-op if statement which can't be true.
>>
>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>> ---
>>   inventory/inv_stobj.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
>> index becac17..84b15d9 100644
>> --- a/inventory/inv_stobj.c
>> +++ b/inventory/inv_stobj.c
>> @@ -976,7 +976,9 @@ stobj_delete_mobj(int fd,
>>
>>           }
>>           free ( mfiles );
>> +/*
>>           if ( dirty );
>> +*/
>>       }
>>
>>       free ( strms );
> 
> Why not remove all the references to dirty in the function?
> Like you said, it doesn't do anything.

Somebody left this and the rest of the commented-out code there
for some documentation reason, it seems, so I figured...

It's not like the rest of the code is a thing
of beauty, and this wart is ruining its perfection.  ;)

I have no idea, honestly.  xfsdump is dropped in from some alien
world.  Somebody @ SGI might know, or could find out by looking
at long-lost history?

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/9 V2] xfsdump: Fix overflow of "question" string in Media_prompt_erase()

[Eric Sandeen]

On 11/13/14 11:38 AM, Christoph Hellwig wrote:
>> +	asprintf( &question, 
>> +		 "overwrite data on media in drive %u?\n",
>>  		 (unsigned int)drivep->d_index );
> 
> Where is the error handling?

Well, ok.

The error handling ahead of this is crap; if the
function returns !ok, we change the media:

                                ok = Media_prompt_overwrite( drivep );
                                if ( intr_allowed && cldmgr_stop_requested( )) {
                                        return RV_INTR;
                                }
                                if ( ! ok ) {
                                        goto changemedia;
                                }

I don't want to try to understand and rework all the ways we can
back out of this if we don't have ~100 bytes available.

So, what would you prefer:

I could make a 110-byte array, and snprintf max 110 bytes to it.
Or I could asprintf, and exit the whole program if it fails.

I'm not trying to make xfsdump perfect, I'm trying to make it
suck less. ;)

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 9/9] xfsdump: fix uninit ackstr in content_mediachange_query()

[Mark Tinguely]

On 11/12/14 12:57, Eric Sandeen wrote:
> Today, this sends an uninitialized ackstr[0] to be mlog'd -
> who knows what we get out of it.  Other places follow this
> "count = 0, string = "\n"" pattern which seemsa bit odd, but
> better than printing uninitialized memory.
>
> To be completely honest, I have no test for this.
>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---
>   restore/content.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/restore/content.c b/restore/content.c
> index c99aed7..bc5b398 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -2915,6 +2915,7 @@ content_mediachange_query( void )
>   			bagp = 0;
>   		}
>   		ackcnt = 0;
> +		ackstr[ ackcnt++ ] = "\n";
>   		dlog_multi_ack( ackstr,
>   				ackcnt );
>   		querycnt = 0;

dlog_multi_ack() with count of 0 will exit without doing anything.

Looks like some conditional code that filled the ackstr array (like the 
other callers) was removed. I vote to pull ackstr, ackcnt and the 
dlog_multi_ack() from this function.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/9] xfsdump: remove pointless if (dirty);

[Mark Tinguely]

On 11/13/14 11:47, Eric Sandeen wrote:
> On 11/13/14 11:35 AM, Mark Tinguely wrote:
>> On 11/12/14 12:57, Eric Sandeen wrote:
>>> "dirty" seems to be a remnant of unfinished code; other bits of
>>> it are there but commented out, so comment out "if (dirty);" as
>>> well, which is a no-op if statement which can't be true.
>>>
>>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>>> ---
>>>    inventory/inv_stobj.c |    2 ++
>>>    1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
>>> index becac17..84b15d9 100644
>>> --- a/inventory/inv_stobj.c
>>> +++ b/inventory/inv_stobj.c
>>> @@ -976,7 +976,9 @@ stobj_delete_mobj(int fd,
>>>
>>>            }
>>>            free ( mfiles );
>>> +/*
>>>            if ( dirty );
>>> +*/
>>>        }
>>>
>>>        free ( strms );
>>
>> Why not remove all the references to dirty in the function?
>> Like you said, it doesn't do anything.
>
> Somebody left this and the rest of the commented-out code there
> for some documentation reason, it seems, so I figured...
>
> It's not like the rest of the code is a thing
> of beauty, and this wart is ruining its perfection.  ;)
>
> I have no idea, honestly.  xfsdump is dropped in from some alien
> world.  Somebody @ SGI might know, or could find out by looking
> at long-lost history?
>
> -Eric

I will see what I can dig up.

We need to start pulling off the warts.

The interactive command line parsing is also damaged, I will find the 
patch and post it.

--Mark.


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 9/9] xfsdump: fix uninit ackstr in content_mediachange_query()

[Eric Sandeen]

On 11/13/14 12:10 PM, Mark Tinguely wrote:
> On 11/12/14 12:57, Eric Sandeen wrote:
>> Today, this sends an uninitialized ackstr[0] to be mlog'd -
>> who knows what we get out of it.  Other places follow this
>> "count = 0, string = "\n"" pattern which seemsa bit odd, but
>> better than printing uninitialized memory.
>>
>> To be completely honest, I have no test for this.
>>
>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>> ---
>>   restore/content.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/restore/content.c b/restore/content.c
>> index c99aed7..bc5b398 100644
>> --- a/restore/content.c
>> +++ b/restore/content.c
>> @@ -2915,6 +2915,7 @@ content_mediachange_query( void )
>>               bagp = 0;
>>           }
>>           ackcnt = 0;
>> +        ackstr[ ackcnt++ ] = "\n";
>>           dlog_multi_ack( ackstr,
>>                   ackcnt );
>>           querycnt = 0;
> 
> dlog_multi_ack() with count of 0 will exit without doing anything.

Oh, yeah, I misread it.  Sorry.  I saw stuff like this in sigint_dialog()

                ackcnt = 0;
                ackstr[ ackcnt++ ] = "\n";
                dlog_multi_ack( ackstr,
                                ackcnt );


and got carried away.

> Looks like some conditional code that filled the ackstr array (like
> the other callers) was removed. I vote to pull ackstr, ackcnt and the
> dlog_multi_ack() from this function.

I wondered about that, but didn't want to go changing things I wasn't
able to test or trivially understand.

However, I guess I'm challenged even in the latter this week.  ;)

Anyway, yeah, just drop this patch, sorry.

Thanks for the review,
-Eric


> --Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs