On Sun, Jun 21, 2015 at 02:27:22AM -0700, Christoph Hellwig wrote:
> This looks sensible to me, but I still can't make sense of the old
> code which just conditionally copied it even after taking a brief
> look at the pre-git history of this code. Does anyone understand why
> the code was like this? Fixing code that seems to have had an
> intention I can't make sense of always feel dangerous.
>
Yeah, I couldn't really make much sense of the original code either. My
reasoning at the time was that the memcpy() seemed superfluous in this
case, as we wouldn't add anything to trans->r_itemq and end up doing the
memcpy() again the next time around.
Taking another look at other xlog_recover_add_item() callsites, there is
this in the xlog_recover_add_to_cont_trans() case:
if (list_empty(&trans->r_itemq)) {
/* finish copying rest of trans header */
xlog_recover_add_item(&trans->r_itemq);
ptr = (xfs_caddr_t) &trans->r_theader +
sizeof(xfs_trans_header_t) - len;
memcpy(ptr, dp, len);
return 0;
}
... which according to the code/comment, seems to imply that the
transaction header could be split across op records..? I'm not terribly
familiar with the log on-disk format. Does this sound sane?
If so, perhaps the patch is wrong and we should only bail if the length
in either of these two cases is obviously invalid (e.g., it exceeds the
size of the full in-core structure). Perhaps there's also more
validation that can occur here: can we assert that this should mean
we're at the end of the operation record in the first short copy
instance? I'll probably have to stare at this some more with regard to
that.
Brian
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
