Re: [PATCH/RFC v2 09/16] Read index-v5

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thomas Gummerer <t.gummerer@xxxxxxxxx> writes:

>> > +	name = (char *)mmap + *dir_offset;
>> > +	beginning = mmap + *dir_table_offset;
>> 
>> Notice how you computed name with pointer arithmetic by first
>> casting mmap (which is "void *") and when computing beginning, you
>> forgot to cast mmap and attempted pointer arithmetic with "void *".
>> The latter does not work and breaks compilation.
>> 
>> The pointer-arith with "void *" is not limited to this function.
> ...
> I've used the type of the respective assignment for now. e.g. i have
> struct cache_header *hdr, so I'm using
> hdr = (struct cache_header *)mmap + x;

You need to be careful when rewriting the above to choose the right
value for 'x' if you go that route (which I wouldn't recommend).

With

    hdr = ptr_add(mmap, x);

you are making "hdr" point at x BYTES beyond mmap, but

    hdr = (struct cache_header *)mmap + x;

means something entirely different, no?  "hdr" points at x entries
of "struct cache_header" beyond mmap (in other words, if mmap[] were
defined as "struct cache_header mmap[]", the above is saying the
same as "hdr = &mmap[x]").

I think the way you casted to compute the value for the "name"
pointer is the (second) right thing to do.  The cast (char *)
applied to "mmap" is about "mmap is a typeless blob of memory I want
to count bytes in.  Give me *dir_offset bytes into that blob".  It
is not tied to the type of LHS (i.e. "name") at all.  The result
then needs to be casted to the type of LHS (i.e. "name"), and in
this case the types happen to be the same, so you do not have to
cast the result of the addition but that is mere luck.

The next line is not so lucky and you would need to say something
like:

    beginning = (uint32_t *)((char *)mmap + *dir_table_offset);

Again, inner cast is about "mmap is a blob counted in bytes", the
outer cast is about type mismatch between a byte-address and LHS of
the assignment.

If mmap variable in this function were not "void *" but something
more sane like "const char *", you wouldn't have to have the inner
cast to begin with, and that is why I said the way you did "name" is
the second right thing.  Then you can write them like

    name = mmap + *dir_offset;
    beginning = (uint32_t *)(mmap + *dir_offset);

After thinking about this, the ptr_add() macro might be the best
solution, even though I originally called it as a band-aid.  We know
mmap is a blob of memory, byte-offset of each component of which we
know about, so we can say

    name = ptr_add(mmap, *dir_offset);
    beginning = ptr_add(mmap, *dir_offset);

Hmmm..


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]