Another fix for UFS with inode number sign propagation

[kib]

Summary

ufs_vnops.c: use unsigned type for newparent inode number in ufs_rename()

Otherwise it is sign-extended into 64bit ino_t on the call to
ufs_dirrewrite().  This causes invalid inode number recorded in the SU
tracking structures (newdirem) and triggers corresponding panics.


ufs_vnops.c: newparent is not bool

Use proper comparision operators when we need to see if newparent was
set to not-zero value.

[kib]

This causes

panic: newdirrem: inum 2147911196 should be 18446744071562495516

2147911196 == 0x8006861c
18446744071562495516 == 0xffffffff8006861c

[olce]

sys/ufs/ufs/ufs_vnops.c

1272

This is still wrong on 32-bit architectures.

1645–1647

While here.

[kib]

sys/ufs/ufs/ufs_vnops.c

1272

In which way it is wrong? Or do you mean that your suggested change is wrong?

Anyway, I state that I do want it the way I provided it in review. u_int (might be uint32_t), not ino_t.

[olce]

sys/ufs/ufs/ufs_vnops.c

1272

ino_t is a uint64_t. I thought we were using 64-bit inode numbers everywhere. But I see that struct direct has a uint32_t d_ino, so I guess UFS is still limited to 32-bit inode numbers? In which case your change is indeed not wrong, although surprising given that involved functions take an ino_t parameter. I'd much prefer inconsistency reduction here, even at the price of handling a 64-bit number on 32-bit architectures (unless we gain something else by doing so that I don't see?).

[olce]

sys/ufs/ufs/ufs_vnops.c

1646–1647

This is the proper equivalent, my previous suggestion is wrong.

[kib]

sys/ufs/ufs/ufs_vnops.c

1272

Inode numbers on UFS are 32bit, this is on-disk format, explicit as in the struct direct, and implicit in the mapping from inode number to inode block number. We should use that in the UFS/FFS code.

In fact doing inode number as uint64_t is probably wrong for inode block number calculation.

[olce]

sys/ufs/ufs/ufs_vnops.c

1272

In fact doing inode number as uint64_t is probably wrong for inode block number calculation.

If there is no UFS plan for an extension to 64-bit inode numbers, indeed. That should of course not block this change.

[kib]

sys/ufs/ufs/ufs_vnops.c

1272

This would be UFS3.
At least I do not plan to work on this in foreseeable future.

[kib]

Simplify one logical op.

[olce]

sys/ufs/ufs/ufs_vnops.c

1272

I don't either. And I don't think mckusick@ does either.

[mckusick]

This change is correct.

[pfg]

sys/ufs/ufs/ufs_vnops.c

1272

FWIW, and sorry for being late ...
There was an effort to move to standard types in UFS (831b1ff7913f), so that should have been unsigned int, or uint32_t

[kib]

sys/ufs/ufs/ufs_vnops.c

1272

Referenced revision removed u_intXX_t. I do not see a reason why u_int contraction would be banned.

[pfg]

sys/ufs/ufs/ufs_vnops.c

1272

It is not formally banned, but the idea was to favor standard types: u_int is in the same category of u_intXX_t. I recall there was another commit that also swept some u_ints but I can't find the log.

In any case ... nevermind, I am not asking for it to be changed, it just looked a little weird.