Derrick Stolee <stolee@xxxxxxxxx> writes:
>> But a silent type casting from size_t to uint64_t done silently by
>> assignment bothers me.
>
> Does this bother you only because its part of the external interface?
> If I understand correctly, uint64_t will always be at least as big
> as size_t, so this doesn't need any protections for overflow or
> anything. Is there something I should be doing before casting?
I am OK to use uint64_t on the caller-facing side, as long as we
explicitly declare that uint64_t ought to be large enough for
everybody. Struct members and variables that are closer to the
on-disk data need to be of sized type to avoid repeating the pain
caused by our early "unsigned long ought to be large enough for
everybody" attitude, but it is nicer to be working with more
abstract types in the layer higher up. And if there is a risk of
truncation in either direction, we should be defensive. That's all.
>>> +int write_chunkfile(struct chunkfile *cf, void *data)
>>> +{
>>> + int i;
>>> + size_t cur_offset = cf->f->offset + cf->f->total;
>>
>> That ought to be off_t, as it is a seek position inside a file
>> (struct hashfile.total is already off_t).
>
> I can use off_t for the other offsets in this computation, but
> cur_offset will be used in hashwrite_be64(), so maybe it is best
> to use uint64_t here?
As I discovered in the later parts, I think off_t makes less sense
than size_t in the context of this design, so size_t is fine, as
long as we keep the "users of chunkfile API work on a mmapped region
of contiguous memory" (which I think is OK). uint64_t is also fine,
as long as this one is an internal implementation detail; iow,
callers of the API do not have to be responsible for casting their
more abstract and platform neutral types down to these fixed-sized
types even if we choose to use uint64_t here.
> The current chunk format API makes the same assumption (ToC comes
> first) but could be adjusted later to let this part of the method
> dynamically compute the chunk sizes and fill a ToC at the end. The
> way to modify this API would be to add a 'flags' parameter.
>
> So far, this has not been necessary, but might be in the future.
Yup, and I am happy with the current design for now. Thanks for
clarifying the thinking behind it.
