Samuel Thibault <samuel.thiba...@gnu.org> writes:
> Maksym Planeta, le Sat 07 Apr 2012 19:51:56 +0300, a écrit :
>> Here is initialization code from pager_alloc():
>> if (INDIRECT_PAGEMAP(size)) {
>> alloc_size = INDIRECT_PAGEMAP_SIZE(size);
>> init_value = (dp_map_t)0;
>>
>> And from pager_extend():
>> for (; i < INDIRECT_PAGEMAP_ENTRIES(new_size); i++)
>> new_mapptr[i].indirect = (dp_map_t)0;
>>
>> As you can see instead of NULL, (dp_map_t)0 is used.
>
> And can be put into pager->map in pager_truncate, ok. I'm however not
> sure we really want to put ifs everywhere. The comment in the truncation
> says
>
> /* We are truncating to a size small enough that it goes to using
> a one-level map. We already have that map, as the first and only
> nonempty element in our indirect map. */
>
> i.e. the code assumes that map[0].indirect is not NULL. I'd say we
> should rather allocate an empty map in such case, to keep the rest of
> the code simple.
>
And what is the alternative for ifs? longjumps and setjumps? Purpose of
this conditions is checking whether map (or submap) is already empty.
>> > There is also an issue with
>> >
>> > + if (!pager->map) {
>> > + invalidate_block (pager_offset);
>> > + goto done;
>> > + }
>> > pager_offset = pager->map[f_page];
>> >
>> > at that point, pager_offset is not initialized yet...
>> >
>>
>> invalidate_block is a macro that sets pager_offset, so, really,
>> pager_offset shouldn't been initialized yet.
>
> It sets the *content* pointed by pager_offset. It does not set the
> pager_offset pointer.
>
pager_offset is not a pointer, it is a union. So setting pager_offset
is the same as setting its content.
>> > Also I don't understand the use of
>> >
>> > + if (no_block (*pager->map))
>> > + goto done;
>> > entry = pager->map[offset];
>> > invalidate_block(pager->map[offset]);
>> >
>> > *pager->map would be pager->map[0], I'd believe that it is independent
>> > from pager->map[offset], and clearing it can only bring issues.
>> >
>>
>> Sorry, didn't get you.
>
> *pager->map is the same as pager->map[0]. AIUI, it has nothing to do
> with pager->map[offset]. So why testing it at all?
>
Hmm... Looks odd, indeed. I think that I should add assertion before
condition, which determines whether mapping indirect or not. Assertion
will check if pager->map is not NULL (I looked through code and it
called only in one function, that should supply pager to
pager_release_offset with non empty map). Finally, I'll remove the
check, you've pointed at.
