[CRIU] Re: [PATCH] service-fd: Rework it to use in hight performance code

[Pavel Emelyanov]

On 09/07/2012 05:08 PM, Cyrill Gorcunov wrote:
> On Fri, Sep 07, 2012 at 04:57:54PM +0400, Pavel Emelyanov wrote:
>> On 09/07/2012 04:44 PM, Cyrill Gorcunov wrote:
>>> On Fri, Sep 07, 2012 at 04:38:21PM +0400, Pavel Emelyanov wrote:
>>>>> +
>>>>> +int is_service_fd(int fd, int type)
>>>>> +{
>>>>> +	if (type > SERVICE_FD_OFF_MIN)
>>>>> +		return fd == get_service_fd(type);
>>>>> +
>>>>> +	return fd > service_fd_rlim_min;
>>>>
>>>> int is_service_fd(int fd)
>>>> {
>>>> 	return fd > serice_fd_rlim_cur - SERVICE_FD_OFF_MAX;
>>>> }
>>>>
>>>
>>> Pavel, this is not enough for my tty restore code. For control
>>> terminals I need to know that fd _is_ CTL_TTY_OFF, not just arbitrary
>>> fd from service range. What happen when we need some other service fd
>>> in file engine?
>>>
>>> Lets do a deal: two helpers
>>>
>>> is_any_service_fd(int fd)
>>> {
>>> 	return fd > serice_fd_rlim_cur - SERVICE_FD_OFF_MAX;
>>> }
>>>
>>> is_service_fd(int fd, int type)
>>> {
>>> 	return fd == get_service_fd(type);
>>> }
>>>
>>> agreed?
>>
>> No. You want to have an ability to check that fd is any service by
> 
> What I really need is to check that fd is service file descriptor
> with mandatory type. I added this _any-fd_ case only because you
> were insisting on form like
> 
> 	return fd > serice_fd_rlim_cur - SERVICE_FD_OFF_MAX;

I was sure that we need to check for fd being _any_ service, regardless of
type, but if you insist, that we do need to check type then OK.

> which is completely useless for my case.
> 
>> passing 0 into type? Then write a normal comment about it, not some
>> strange hint about "change this MIN carefully".
> 
> As to 0 in type -- I think we should simply add BUG_ON here,
> since it's not supposed to be called from crtools code with
> unknown type.
> 
>>
>> And there's no need in TWO global variables _cur and _min.
> 
> There is no need, but it eliminates need to do substraction. I'll drop it.
> 
> 	Cyrill
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://openvz.org/mailman/listinfo/criu
> .
>