Fix i586 size_t compiler error.

[gonzoleeman]

This fix silences a compiler complaint on i586 (32-bit)
in handle_writesame(). A similar fix is done in handle_format_unit(),
since the code is very similar.

Changes since v1:

• removed git merges from commit
• added fix in handle_format_unit()

Original-fix-by: David Disseldorp [email protected]

[gonzoleeman]

Try 2.

[mikechristie]

A couple lines below this we do:

length = round_up(length, max_xfer_length);

then pass length to tcmur_cmd_state_init which wants a size_t. Will you get another error about passing a unin64_t to a size_t now?

[mikechristie]

This PR is also trying to fix the same issue:

#603

[gonzoleeman]

This PR is also trying to fix the same issue:

#603

Honestly, I kind of like that better. The size mismatches in the code (e.g. going to/from int/uint64_t/size_t) are numerous and kind of a can of worms to start fixing IMHO.

@ddiss what do you think (since you initiated the fix I submitted)?

[mikechristie]

Honestly, I kind of like that better. The size mismatches in the code (e.g. going to/from int/uint64_t/size_t) are numerous and kind of a can of worms to start fixing IMHO.

I didn't merge that patch, because I wanted to fix it properly.

However, it seems like the only time these get reported are just for builds and not actual users of 32 bit setups.

Are you guys seeing 32 bit tcmu runner users? We had one a long while back and they hit a issue with the device size:

#360

[gonzoleeman]

Honestly, I kind of like that better. The size mismatches in the code (e.g. going to/from int/uint64_t/size_t) are numerous and kind of a can of worms to start fixing IMHO.

I didn't merge that patch, because I wanted to fix it properly.

IMHO that would take a bit of work, and for what? For 32-bit? Not sure it's worth it.

However, it seems like the only time these get reported are just for builds and not actual users of 32 bit setups.

Are you guys seeing 32 bit tcmu runner users? We had one a long while back and they hit a issue with the device size:

#360

No. We actually have never had a 32-bit issue. This came up as a build issue for our Factory build (Tumbleweed), and frankly I doubt anyone loads that on a 32-bit system. I actually just let the 32-bit build fail for quite a while, and David was the one that suggested fixing it.

[ddiss]

Honestly, I kind of like that better. The size mismatches in the code (e.g. going to/from int/uint64_t/size_t) are numerous and kind of a can of worms to start fixing IMHO.

tcmu_dev_get_max_xfer_len() returns a uint32_t and tcmu_lba_to_byte() returns a uint64_t, so I think using a uint64_t makes more sense than casting to size_t for min() here.

I didn't merge that patch, because I wanted to fix it properly.

IMHO that would take a bit of work, and for what? For 32-bit? Not sure it's worth it.

However, it seems like the only time these get reported are just for builds and not actual users of 32 bit setups.
Are you guys seeing 32 bit tcmu runner users? We had one a long while back and they hit a issue with the device size:
#360

No. We actually have never had a 32-bit issue. This came up as a build issue for our Factory build (Tumbleweed), and frankly I doubt anyone loads that on a 32-bit system. I actually just let the 32-bit build fail for quite a while, and David was the one that suggested fixing it.

This was just a build regression, I agree that we probably don't have many 32-bit users, but keep in mind that openSUSE carries ARMv7 ports, in addition to i586.

[mikechristie]

Honestly, I kind of like that better. The size mismatches in the code (e.g. going to/from int/uint64_t/size_t) are numerous and kind of a can of worms to start fixing IMHO.

I didn't merge that patch, because I wanted to fix it properly.

IMHO that would take a bit of work, and for what? For 32-bit? Not sure it's worth it.

Maybe we are talking about different things. From my perspective if we are not supporting it then why even waste time on these build issues? If we are supporting it then we could end up having a user and if we prevent a data corruption, then it's worth it for me.

I'm not sure it's that bad. For example, for the WS case, lba_cnt can be up to 32 bits but we then use bytes internally so that gets multiplied so that is why we used u64 in some places. Maybe for 32 bit setups we need to at least set VPD_MAX_WRITE_SAME_LENGTH lower so we will always be within a 32bit value after the byte conversion. Then add a check that the incoming command obeys the reported limit.

[mikechristie]

I'm not sure it's that bad.

Doh. I think to support 32bit setups we probably need a lot of other fixes. Due to some other 32 bit setup bug that limits the device size we currently do not need stuff like the lowered WS size and limit check since we could never have a device that is large enough.

So I guess my concern is more general and do we want to even support 32 bit setups or just add something so it does not build.