[lttng-dev] [LTTNG-TOOLS PATCH v2] ABI with support for compat 32/64 bits

David Goulet dgoulet at efficios.com
Mon Oct 1 14:43:41 EDT 2012

Hash: SHA512

Julien Desfossez:
>>>>>> +/* + * This flag indicates if lttng-tools must use the
>>>>>> new or the old kernel ABI + * (without compat support for
>>>>>> 32/64 bits). It is set by + * kernctl_tracer_version()
>>>>> Hrm. I don't like that this is only set by 
>>>>> kernctl_tracer_version(). What if, for an unforeseen
>>>>> reason (e.g. code change), sessiond starts using "create
>>>>> session" before checking the version ? The change you
>>>>> propose assumes a behavior on the sessiond side that is not
>>>>> cast in stone (no ABI requires it), so it might change.
>>>>> This brings coupling between this otherwise self-contained
>>>>> wrapper and the entire sessiond code base, which I don't
>>>>> like.
>>>>> We should put the check in a wrapper macro around the every
>>>>> new ioctl call.
>>>>> e.g.
>>>>> /* * Cache whether we need to use the old or new ABI. */
>>>>> static lttng_kernel_use_old_abi = -1;
>>>>> if (lttng_kernel_use_old_abi == -1) { ret = ioctl(fd,
>>>>> newname, args); if (!ret) { lttng_kernel_use_old_abi = 0; }
>>>>> } else { if (!lttng_kernel_use_old_abi) { ret = ioctl(fd,
>>>>> oldname, args); } else { ret = ioctl(fd, newname, args); }
>>>>> } return ret;
>>>>> Thoughts ?
>>>> Ok I will add this wrapper, I am wondering if we could just
>>>> limit to these ioctl since they are the only one called on
>>>> the "top-level" fd (/proc/lttng) instead of adding it
>>>> All other ioctl require at least a session so is it OK if I
>>>> limit the test to only these ?
>>> yes, it makes sense. And given that this code is very much 
>>> localized, I don't think it would be worth the effort to create
>>> a macro wrapper. Duplicating the checks at each 5 sites, all
>>> within the same file, seems good enough. But it's David's
>>> call.
>> I don't know... I can easily see some other use cases in the
>> future that requires us to wrap the ioctl according to the
>> kernel.
>> I remember having this discussion way back when we started
>> lttng-tools code and here we are almost three years later fixing
>> the "ioctl wrapper issue" :P.
>> I don't think adding a macro is a lot of work and it will be
>> really easier for us to scale and adapt over time. Please, if I'm
>> wrong, speak now or shut up to the end of eternity! :P
> For this particular case, we need to make special treatment when
> the ioctl takes an argument depending on the ABI version (alloc and
> assign the old struct values from the new one), so we won't be able
> to call a generic wrapper (except if we do this treatment all the
> time which I doubt we want). For ioctls that don't take an argument
> the wrapper is easy though.
> So I think I should do a wrapper like compat_ioctl_no_arg(int fd,
> unsigned long oldname, unsigned long newname) and for the ioctls
> that take an argument make the checks locally.

Hmmm it's that or a compat call for each ioctl listed above... or
using va_list but I really don't like that :P

Either way works for me. Mathieu?


> Thoughts ?
> Thanks,
> Julien


More information about the lttng-dev mailing list