> > +{
> > + ASSERT(from >= 0 && from <= 1000);
> > + ASSERT(to >= 0 && to <= 1000);
> > + ASSERT(numptrs >= 0);
>
> Those numbers are not safe. I plucked them out of thin air to verify
> validity on 4k block size filesystem which had (IIRC) a max of about
> 500 ptrs to a block. It was throwaway debug code to find a problem.
> Larger block sizes can well exceed 1000. So realistically, the only
> valid assert there is this one:
>
> ASSERT(numptrs >= 0);
Yeah, I've actually managed to trigger it now. The >=0 checks
for from and to still make sense, although they might be overkill.
> How about:
>
> union xfs_btree_ptr *pp;
> xfs_caddr_t *block = XFS_BUF_TO_BLOCK(bp);
> xfs_caddr_t start; /* first byte offset logged */
> xfs_caddr_t end; /* last byte offset logged */
>
> pp = cur->bc_ops->ptr_addr(cur, 1, XFS_BUF_TO_BLOCK(bp));
>
> if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
> __be64 *lpp = &pp->l;
>
> start = (xfs_caddr_t)&lpp[first - 1] - block;
> end = ((xfs_caddr_t)&lpp[last] - 1) - block;
> } else {
> __be32 *spp = &pp->s;
>
> start = (xfs_caddr_t)&spp[first - 1] - block;
> end = ((xfs_caddr_t)&spp[last] - 1) - block;
> }
>
> xfs_trans_log_buf(cur->bc_tp, bp, (int)start, (int)end);
>
> That makes it much easier to read (to me, anyway).
Yes, absolutely. And there's also another set of useless braces.
I've also applied a similar cleanup to the log_keys and log_recs
implementations.
> > + XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
> > + XFS_BTREE_TRACE_ARGBI(cur, bp, fields);
> > +
> > + if (bp) {
> > + xfs_btree_offsets(fields,
> > + (cur->bc_flags & XFS_BTREE_LONG_PTRS) ?
> > + loffsets : soffsets,
> ^^
> Some stray whitespace there.
Fixed.
> > + XFS_BB_NUM_BITS, &first, &last);
> > + xfs_trans_log_buf(cur->bc_tp, bp, first, last);
> > + } else {
> > + /* XXX(hch): maybe factor out into a method? */
> > + xfs_trans_log_inode(cur->bc_tp, cur->bc_private.b.ip,
> > + XFS_ILOG_FBROOT(cur->bc_private.b.whichfork));
>
> I don't think it is necessary at this point.
It's the only leakage of the detailed inode root implementation into
the generic code, so I'm still wondering whether a method would be
better.
|