| Message ID | 155259758358.31886.2042156413647033104.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfsprogs-5.0: fix various problems | expand |
On 3/14/19 4:06 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Fix some uninitialized variable warnings because ASSERT disappears if > DEBUG is not defined. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Fixes: commit 5857dce9e ("xfs_repair: check and repair quota metadata") which was mine ;) But, I wonder if we should really just happily carry on w/ an invalid quota type 0 on a non-debug build. I don't know how that will end. Maybe we should just switch it to a hard assert(0) because it really can only happen on a programming error today. > --- > repair/dinode.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > > diff --git a/repair/dinode.c b/repair/dinode.c > index f670bf87..c0a56daa 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -1176,8 +1176,8 @@ process_quota_inode( > struct xfs_buf *bp; > xfs_filblks_t dqchunklen; > uint dqperchunk; > - int quota_type; > - char *quota_string; > + int quota_type = 0; > + char *quota_string = NULL; > xfs_dqid_t dqid; > xfs_fileoff_t qbno; > int i; >
On Tue, Mar 26, 2019 at 02:56:02PM -0500, Eric Sandeen wrote: > On 3/14/19 4:06 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Fix some uninitialized variable warnings because ASSERT disappears if > > DEBUG is not defined. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > Fixes: commit 5857dce9e ("xfs_repair: check and repair quota metadata") > > which was mine ;) > > But, I wonder if we should really just happily carry on w/ an invalid > quota type 0 on a non-debug build. I don't know how that will end. > > Maybe we should just switch it to a hard assert(0) because it really > can only happen on a programming error today. Yeah, we ought to be more defensive about that, since in theory we could introduce a subtle bug some day where we validate a corrupt dquot with flags == 0. /me kinda wonders why we define away the asserts for repair, since it's not like continuing under invalid premises in a repair tool is a good idea. --D > > > --- > > repair/dinode.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/repair/dinode.c b/repair/dinode.c > > index f670bf87..c0a56daa 100644 > > --- a/repair/dinode.c > > +++ b/repair/dinode.c > > @@ -1176,8 +1176,8 @@ process_quota_inode( > > struct xfs_buf *bp; > > xfs_filblks_t dqchunklen; > > uint dqperchunk; > > - int quota_type; > > - char *quota_string; > > + int quota_type = 0; > > + char *quota_string = NULL; > > xfs_dqid_t dqid; > > xfs_fileoff_t qbno; > > int i; > >
On 3/14/19 4:06 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Fix some uninitialized variable warnings because ASSERT disappears if > DEBUG is not defined. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > repair/dinode.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > > diff --git a/repair/dinode.c b/repair/dinode.c > index f670bf87..c0a56daa 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -1176,8 +1176,8 @@ process_quota_inode( > struct xfs_buf *bp; > xfs_filblks_t dqchunklen; > uint dqperchunk; > - int quota_type; > - char *quota_string; > + int quota_type = 0; > + char *quota_string = NULL; > xfs_dqid_t dqid; > xfs_fileoff_t qbno; > int i; > well, I was concerned that if we get here with an invalid inode type, we'll use an invalid quota type, and <unknown> will happen. But we're relying on the case statement which calls us, which has only valid types. ... I guess we could s/ASSERT/assert/ so it's there, or just don't worry be happy. There doesn't seem to be much appetite for spot-fixing this, so Reviewed-by: Eric Sandeen <sandeen@redhat.com>
diff --git a/repair/dinode.c b/repair/dinode.c index f670bf87..c0a56daa 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -1176,8 +1176,8 @@ process_quota_inode( struct xfs_buf *bp; xfs_filblks_t dqchunklen; uint dqperchunk; - int quota_type; - char *quota_string; + int quota_type = 0; + char *quota_string = NULL; xfs_dqid_t dqid; xfs_fileoff_t qbno; int i;