Code review comment for lp://qastaging/~3v1n0/unity/lim

Revision history for this message
Marco Trevisan (TreviƱo) (3v1n0) wrote :

First of all, using pipe() is just done for convenience (to quickly get a writable fd), you can do that also using open with any valid temporary file (but not /dev/null, since it's a special file).

> > Also, since the IndicatorObjectEntry's are only structs and not GObject's,
> this is the only way we have to check if we're really working with a valid
> one.
>
> There's no way to know that, and the pipe() trick does not ensure that
> either. If the IndicatorObjectEntry is allocated with the slice
> allocated (fx.) it'll still be valid memory even after freed, but
> might contain garbage.

Yes, but... The fact is: if the pointer we found is pointing to an area of memory where we have no rights, without using this trick we'd crash. Using this trick instead, if that pointer is not on a valid area, write won't be able to read from it and instead of crashing it will give an EFAULT error.

Now, if that area is readable, write won't give any error, but at that point INDICATOR_IS_OBJECT will be able to check the pointed object without crashing, since it's in a valid area...
I've made tests locally to see if this was working on a manually allocated area (with garbage too) or even checking "self", and all the results are with me, because if INDICATOR_IS_OBJECT (and so g_type_check_instance()) will fail, is very hard that entry is valid.

> I am not entirely understanding the reasoning here. Is it that
> creating a pipe and checking the write is cheaper than a hashtable
> lookup? I doubt it as pipe(), write(), and close() requires a few mode
> switches. And arguably I'd consider the hashtable trick more safe.

Well, on this side I think that is hard to be cheaper than this... Especially on memory side.
Also, that method is not crash-free by design... Here we can be sure that no crash will happen.

> (although the pipe trick is neat, I am also wondering if there
> wouldn't be some lower level call that checked this directly..? not
> arguing in the favour of that though :-))

No... I guess we should improve glib! :)

« Back to merge proposal