| Message ID | 8062b2d0-3fbb-0240-d5dd-c7bfb452f0b3@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | xfs_db: more type_f cleanups | expand |
On Thu, Aug 20, 2020 at 07:09:45PM -0500, Eric Sandeen wrote: > Right now there are 3 cases to type_f: inode type, type with fields, > and a default. The first two were added to address issues with handling > V5 metadata. > > The first two already use some version of set_cur, which handles all > of the validation etc. There's no reason to leave the open-coded bits > at the end, just send every non-inode type through set_cur and be done > with it. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > io.c | 28 +++++----------------------- > 1 file changed, 5 insertions(+), 23 deletions(-) > > diff --git a/db/io.c b/db/io.c > index 884da599..235191f5 100644 > --- a/db/io.c > +++ b/db/io.c > @@ -603,33 +603,15 @@ set_iocur_type( > iocur_top->boff / mp->m_sb.sb_inodesize); > ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), agino); > set_cur_inode(ino); > - return; > - } > - > - /* adjust buffer size for types with fields & hence fsize() */ > - if (type->fields) { > - int bb_count; /* type's size in basic blocks */ > + } else { Two spaces ^^ between the else and the paren. I also wonder, why not leave the "return;" at the end of the "if (type->typnm == TYP_INODE)" part and then you can reduce the indenting level of the rest of the function? <shrug> maintainer's prerogative on that one though. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> /me also wonders why dquots don't get the same "save the buffer offset" treatment as inodes, but that's a separate question. :) --D > + int bb_count = 1; /* type's size in basic blocks */ > > - bb_count = BTOBB(byteize(fsize(type->fields, > + /* adjust buffer size for types with fields & hence fsize() */ > + if (type->fields) > + bb_count = BTOBB(byteize(fsize(type->fields, > iocur_top->data, 0, 0))); > set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL); > } > - iocur_top->typ = type; > - > - /* verify the buffer if the type has one. */ > - if (!bp) > - return; > - if (!type->bops) { > - bp->b_ops = NULL; > - bp->b_flags |= LIBXFS_B_UNCHECKED; > - return; > - } > - if (!(bp->b_flags & LIBXFS_B_UPTODATE)) > - return; > - bp->b_error = 0; > - bp->b_ops = type->bops; > - bp->b_ops->verify_read(bp); > - bp->b_flags &= ~LIBXFS_B_UNCHECKED; > } > > static void >
On 8/21/20 9:52 AM, Darrick J. Wong wrote: > On Thu, Aug 20, 2020 at 07:09:45PM -0500, Eric Sandeen wrote: >> Right now there are 3 cases to type_f: inode type, type with fields, >> and a default. The first two were added to address issues with handling >> V5 metadata. >> >> The first two already use some version of set_cur, which handles all >> of the validation etc. There's no reason to leave the open-coded bits >> at the end, just send every non-inode type through set_cur and be done >> with it. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> io.c | 28 +++++----------------------- >> 1 file changed, 5 insertions(+), 23 deletions(-) >> >> diff --git a/db/io.c b/db/io.c >> index 884da599..235191f5 100644 >> --- a/db/io.c >> +++ b/db/io.c >> @@ -603,33 +603,15 @@ set_iocur_type( >> iocur_top->boff / mp->m_sb.sb_inodesize); >> ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), agino); >> set_cur_inode(ino); >> - return; >> - } >> - >> - /* adjust buffer size for types with fields & hence fsize() */ >> - if (type->fields) { >> - int bb_count; /* type's size in basic blocks */ >> + } else { > > Two spaces ^^ between the else and the paren. > > I also wonder, why not leave the "return;" at the end of the > "if (type->typnm == TYP_INODE)" part and then you can reduce the > indenting level of the rest of the function? Oh, that might be nice. I'll see how that looks. > <shrug> maintainer's prerogative on that one though. > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > /me also wonders why dquots don't get the same "save the buffer offset" > treatment as inodes, but that's a separate question. :) hmmmm good question tho. Thanks for the reviews, -Eric > --D > >> + int bb_count = 1; /* type's size in basic blocks */ >> >> - bb_count = BTOBB(byteize(fsize(type->fields, >> + /* adjust buffer size for types with fields & hence fsize() */ >> + if (type->fields) >> + bb_count = BTOBB(byteize(fsize(type->fields, >> iocur_top->data, 0, 0))); >> set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL); >> } >> - iocur_top->typ = type; >> - >> - /* verify the buffer if the type has one. */ >> - if (!bp) >> - return; >> - if (!type->bops) { >> - bp->b_ops = NULL; >> - bp->b_flags |= LIBXFS_B_UNCHECKED; >> - return; >> - } >> - if (!(bp->b_flags & LIBXFS_B_UPTODATE)) >> - return; >> - bp->b_error = 0; >> - bp->b_ops = type->bops; >> - bp->b_ops->verify_read(bp); >> - bp->b_flags &= ~LIBXFS_B_UNCHECKED; >> } >> >> static void >> >
diff --git a/db/io.c b/db/io.c index 884da599..235191f5 100644 --- a/db/io.c +++ b/db/io.c @@ -603,33 +603,15 @@ set_iocur_type( iocur_top->boff / mp->m_sb.sb_inodesize); ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), agino); set_cur_inode(ino); - return; - } - - /* adjust buffer size for types with fields & hence fsize() */ - if (type->fields) { - int bb_count; /* type's size in basic blocks */ + } else { + int bb_count = 1; /* type's size in basic blocks */ - bb_count = BTOBB(byteize(fsize(type->fields, + /* adjust buffer size for types with fields & hence fsize() */ + if (type->fields) + bb_count = BTOBB(byteize(fsize(type->fields, iocur_top->data, 0, 0))); set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL); } - iocur_top->typ = type; - - /* verify the buffer if the type has one. */ - if (!bp) - return; - if (!type->bops) { - bp->b_ops = NULL; - bp->b_flags |= LIBXFS_B_UNCHECKED; - return; - } - if (!(bp->b_flags & LIBXFS_B_UPTODATE)) - return; - bp->b_error = 0; - bp->b_ops = type->bops; - bp->b_ops->verify_read(bp); - bp->b_flags &= ~LIBXFS_B_UNCHECKED; } static void
Right now there are 3 cases to type_f: inode type, type with fields, and a default. The first two were added to address issues with handling V5 metadata. The first two already use some version of set_cur, which handles all of the validation etc. There's no reason to leave the open-coded bits at the end, just send every non-inode type through set_cur and be done with it. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- io.c | 28 +++++----------------------- 1 file changed, 5 insertions(+), 23 deletions(-)