Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

GIT version control wrote:
> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
> index 11f47c0..a896031 100644
> --- a/ksrc/skins/posix/mq.c
> +++ b/ksrc/skins/posix/mq.c
> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>  	return 0;
>  
>        unlock_and_error:
> +	xnfree(binding):
>  	xnlock_put_irqrestore(&nklock, s);
>  	return err;
>  }

Ok. Will pull. But I really need to fix that.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

Gilles Chanteperdrix wrote:
> GIT version control wrote:
>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>> index 11f47c0..a896031 100644
>> --- a/ksrc/skins/posix/mq.c
>> +++ b/ksrc/skins/posix/mq.c
>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>  	return 0;
>>  
>>        unlock_and_error:
>> +	xnfree(binding):
>>  	xnlock_put_irqrestore(&nklock, s);
>>  	return err;
>>  }
> 
> Ok. Will pull. But I really need to fix that.
> 

Ack - now that I see it myself.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> GIT version control wrote:
>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>> index 11f47c0..a896031 100644
>>> --- a/ksrc/skins/posix/mq.c
>>> +++ b/ksrc/skins/posix/mq.c
>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>  	return 0;
>>>  
>>>        unlock_and_error:
>>> +	xnfree(binding):
>>>  	xnlock_put_irqrestore(&nklock, s);
>>>  	return err;
>>>  }
>> Ok. Will pull. But I really need to fix that.
>>
> 
> Ack - now that I see it myself.
> 

I fixed this in my branch and added another patch to transform EIDRM
into EBADF when selecting a (half-)deleted RTDM device. Please merge.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> GIT version control wrote:
>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>> index 11f47c0..a896031 100644
>>>> --- a/ksrc/skins/posix/mq.c
>>>> +++ b/ksrc/skins/posix/mq.c
>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>  	return 0;
>>>>  
>>>>        unlock_and_error:
>>>> +	xnfree(binding):
>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>  	return err;
>>>>  }
>>> Ok. Will pull. But I really need to fix that.
>>>
>> Ack - now that I see it myself.
>>
> 
> I fixed this in my branch and added another patch to transform EIDRM
> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
> 

Wait! When the sync object behind some file descriptor is deleted but
the descriptor itself is still existing, we rather have to return that
fd signaled from select() instead of letting the call fail. I beed to
look into this again.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> GIT version control wrote:
>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>> index 11f47c0..a896031 100644
>>>>> --- a/ksrc/skins/posix/mq.c
>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>  	return 0;
>>>>>  
>>>>>        unlock_and_error:
>>>>> +	xnfree(binding):
>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>  	return err;
>>>>>  }
>>>> Ok. Will pull. But I really need to fix that.
>>>>
>>> Ack - now that I see it myself.
>>>
>> I fixed this in my branch and added another patch to transform EIDRM
>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>
> 
> Wait! When the sync object behind some file descriptor is deleted but
> the descriptor itself is still existing, we rather have to return that
> fd signaled from select() instead of letting the call fail. I beed to
> look into this again.

It looks to me like a transitory state, we can wait for the sync object
to be deleted to have the fd destructor signaled. It should not be long.

> 
> Jan
> 


-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> GIT version control wrote:
>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>> index 11f47c0..a896031 100644
>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>  	return 0;
>>>>>>  
>>>>>>        unlock_and_error:
>>>>>> +	xnfree(binding):
>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>  	return err;
>>>>>>  }
>>>>> Ok. Will pull. But I really need to fix that.
>>>>>
>>>> Ack - now that I see it myself.
>>>>
>>> I fixed this in my branch and added another patch to transform EIDRM
>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>
>> Wait! When the sync object behind some file descriptor is deleted but
>> the descriptor itself is still existing, we rather have to return that
>> fd signaled from select() instead of letting the call fail. I beed to
>> look into this again.
> 
> It looks to me like a transitory state, we can wait for the sync object
> to be deleted to have the fd destructor signaled. It should not be long.

That's not an issue of waiting for this. See e.g. TCP: peer closes
connection -> internal sync objects will be destroyed (to make
read/write fail). But the fd will remain valid until the local side
closes it as well.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Jan Kiszka wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> GIT version control wrote:
>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>> index 11f47c0..a896031 100644
>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>  	return 0;
>>>>>>>  
>>>>>>>        unlock_and_error:
>>>>>>> +	xnfree(binding):
>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>  	return err;
>>>>>>>  }
>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>
>>>>> Ack - now that I see it myself.
>>>>>
>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>
>>> Wait! When the sync object behind some file descriptor is deleted but
>>> the descriptor itself is still existing, we rather have to return that
>>> fd signaled from select() instead of letting the call fail. I beed to
>>> look into this again.
>> It looks to me like a transitory state, we can wait for the sync object
>> to be deleted to have the fd destructor signaled. It should not be long.
> 
> That's not an issue of waiting for this. See e.g. TCP: peer closes
> connection -> internal sync objects will be destroyed (to make
> read/write fail). But the fd will remain valid until the local side
> closes it as well.

It looks to me like this is going to complicate things a lot, and will
be a source of regressions. Why can we not have sync objects be
destroyed when the fd is really destroyed and use a status bit of some
kind to signal read/write that the fd was closed by peer?

> 
> Jan
> 


-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Jan Kiszka wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> GIT version control wrote:
>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>  	return 0;
>>>>>>>>  
>>>>>>>>        unlock_and_error:
>>>>>>>> +	xnfree(binding):
>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>  	return err;
>>>>>>>>  }
>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>
>>>>>> Ack - now that I see it myself.
>>>>>>
>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>
>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>> the descriptor itself is still existing, we rather have to return that
>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>> look into this again.
>>> It looks to me like a transitory state, we can wait for the sync object
>>> to be deleted to have the fd destructor signaled. It should not be long.
>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>> connection -> internal sync objects will be destroyed (to make
>> read/write fail). But the fd will remain valid until the local side
>> closes it as well.
> 
> It looks to me like this is going to complicate things a lot, and will
> be a source of regressions. Why can we not have sync objects be
> destroyed when the fd is really destroyed and use a status bit of some
> kind to signal read/write that the fd was closed by peer?

It is way easier and more consistent to unblock reader and writers via
destroying the sync object than to signal it and add tests for specific
states to detect that. Keep in mind that this pattern is in use even
without select support. Diverging from it just to add select awareness
to some driver would be a step back.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>> the descriptor itself is still existing, we rather have to return that
>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>> look into this again.
>>>> It looks to me like a transitory state, we can wait for the sync object
>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>> connection -> internal sync objects will be destroyed (to make
>>> read/write fail). But the fd will remain valid until the local side
>>> closes it as well.
>> It looks to me like this is going to complicate things a lot, and will
>> be a source of regressions. Why can we not have sync objects be
>> destroyed when the fd is really destroyed and use a status bit of some
>> kind to signal read/write that the fd was closed by peer?
> 
> It is way easier and more consistent to unblock reader and writers via
> destroying the sync object than to signal it and add tests for specific
> states to detect that. Keep in mind that this pattern is in use even
> without select support. Diverging from it just to add select awareness
> to some driver would be a step back.
> 

First draft of a fix that so far does what it is supposed to do,
comments welcome:

diff --git a/ksrc/skins/posix/syscall.c b/ksrc/skins/posix/syscall.c
index 959b61c..4e46cb6 100644
--- a/ksrc/skins/posix/syscall.c
+++ b/ksrc/skins/posix/syscall.c
@@ -2298,24 +2298,30 @@ static int select_bind_one(struct xnselector *selector, unsigned type, int fd)
 }
 
 static int select_bind_all(struct xnselector *selector,
-			   fd_set *fds[XNSELECT_MAX_TYPES], int nfds)
+			   fd_set *in_fds[XNSELECT_MAX_TYPES],
+			   fd_set *out_fds[XNSELECT_MAX_TYPES], int nfds)
 {
 	unsigned fd, type;
+	int pending = 0;
 	int err;
 
 	for (type = 0; type < XNSELECT_MAX_TYPES; type++) {
-		fd_set *set = fds[type];
+		fd_set *set = in_fds[type];
 		if (set)
 			for (fd = find_first_bit(set->fds_bits, nfds);
 			     fd < nfds;
 			     fd = find_next_bit(set->fds_bits, nfds, fd + 1)) {
 				err = select_bind_one(selector, type, fd);
-				if (err)
-					return err;
+				if (err) {
+					if (err != -EIDRM)
+						return err;
+					__FD_SET(fd, out_fds[type]);
+					pending++;
+				}
 			}
 	}
 
-	return 0;
+	return pending;
 }
 
 /* int select(int, fd_set *, fd_set *, fd_set *, struct timeval *) */
@@ -2387,7 +2393,7 @@ static int __select(struct pt_regs *regs)
 
 		/* Bind directly the file descriptors, we do not need to go
 		   through xnselect returning -ECHRNG */
-		if ((err = select_bind_all(selector, in_fds, nfds)))
+		if ((err = select_bind_all(selector, in_fds, out_fds, nfds)))
 			return err;
 	}
 
@@ -2395,7 +2401,8 @@ static int __select(struct pt_regs *regs)
 		err = xnselect(selector, out_fds, in_fds, nfds, timeout, mode);
 
 		if (err == -ECHRNG) {
-			int err = select_bind_all(selector, out_fds, nfds);
+			int err = select_bind_all(selector, out_fds, out_fds,
+						  nfds);
 			if (err)
 				return err;
 		}

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>>> the descriptor itself is still existing, we rather have to return that
>>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>>> look into this again.
>>>>> It looks to me like a transitory state, we can wait for the sync object
>>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>>> connection -> internal sync objects will be destroyed (to make
>>>> read/write fail). But the fd will remain valid until the local side
>>>> closes it as well.
>>> It looks to me like this is going to complicate things a lot, and will
>>> be a source of regressions. Why can we not have sync objects be
>>> destroyed when the fd is really destroyed and use a status bit of some
>>> kind to signal read/write that the fd was closed by peer?
>> It is way easier and more consistent to unblock reader and writers via
>> destroying the sync object than to signal it and add tests for specific
>> states to detect that. Keep in mind that this pattern is in use even
>> without select support. Diverging from it just to add select awareness
>> to some driver would be a step back.
>>
> 
> First draft of a fix that so far does what it is supposed to do,
> comments welcome:
> 
> diff --git a/ksrc/skins/posix/syscall.c b/ksrc/skins/posix/syscall.c
> index 959b61c..4e46cb6 100644
> --- a/ksrc/skins/posix/syscall.c
> +++ b/ksrc/skins/posix/syscall.c
> @@ -2298,24 +2298,30 @@ static int select_bind_one(struct xnselector *selector, unsigned type, int fd)
>  }
>  
>  static int select_bind_all(struct xnselector *selector,
> -			   fd_set *fds[XNSELECT_MAX_TYPES], int nfds)
> +			   fd_set *in_fds[XNSELECT_MAX_TYPES],
> +			   fd_set *out_fds[XNSELECT_MAX_TYPES], int nfds)
>  {
>  	unsigned fd, type;
> +	int pending = 0;
>  	int err;
>  
>  	for (type = 0; type < XNSELECT_MAX_TYPES; type++) {
> -		fd_set *set = fds[type];
> +		fd_set *set = in_fds[type];
>  		if (set)
>  			for (fd = find_first_bit(set->fds_bits, nfds);
>  			     fd < nfds;
>  			     fd = find_next_bit(set->fds_bits, nfds, fd + 1)) {
>  				err = select_bind_one(selector, type, fd);
> -				if (err)
> -					return err;
> +				if (err) {
> +					if (err != -EIDRM)
> +						return err;
> +					__FD_SET(fd, out_fds[type]);
> +					pending++;
> +				}
>  			}
>  	}
>  
> -	return 0;
> +	return pending;
>  }
>  
>  /* int select(int, fd_set *, fd_set *, fd_set *, struct timeval *) */
> @@ -2387,7 +2393,7 @@ static int __select(struct pt_regs *regs)
>  
>  		/* Bind directly the file descriptors, we do not need to go
>  		   through xnselect returning -ECHRNG */
> -		if ((err = select_bind_all(selector, in_fds, nfds)))
> +		if ((err = select_bind_all(selector, in_fds, out_fds, nfds)))
>  			return err;
>  	}
>  
> @@ -2395,7 +2401,8 @@ static int __select(struct pt_regs *regs)
>  		err = xnselect(selector, out_fds, in_fds, nfds, timeout, mode);
>  
>  		if (err == -ECHRNG) {
> -			int err = select_bind_all(selector, out_fds, nfds);
> +			int err = select_bind_all(selector, out_fds, out_fds,
> +						  nfds);

Mmh, this is probably no good idea, need to truly split in (aka "no yet
bound") and out (aka "pending") sets here as well.

>  			if (err)
>  				return err;
>  		}

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> GIT version control wrote:
>>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>>  	return 0;
>>>>>>>>>  
>>>>>>>>>        unlock_and_error:
>>>>>>>>> +	xnfree(binding):
>>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>  	return err;
>>>>>>>>>  }
>>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>>
>>>>>>> Ack - now that I see it myself.
>>>>>>>
>>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>>
>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>> the descriptor itself is still existing, we rather have to return that
>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>> look into this again.
>>>> It looks to me like a transitory state, we can wait for the sync object
>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>> connection -> internal sync objects will be destroyed (to make
>>> read/write fail). But the fd will remain valid until the local side
>>> closes it as well.
>> It looks to me like this is going to complicate things a lot, and will
>> be a source of regressions. Why can we not have sync objects be
>> destroyed when the fd is really destroyed and use a status bit of some
>> kind to signal read/write that the fd was closed by peer?
> 
> It is way easier and more consistent to unblock reader and writers via
> destroying the sync object than to signal it and add tests for specific
> states to detect that. Keep in mind that this pattern is in use even
> without select support. Diverging from it just to add select awareness
> to some driver would be a step back.

Ok. As you wish. But in that case, please provide a unit test case which
we can run on all architectures to validate your modifications. We will
not put this test case in xenomai repository but will create a
repository for test cases later on.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> GIT version control wrote:
>>>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>>>  	return 0;
>>>>>>>>>>  
>>>>>>>>>>        unlock_and_error:
>>>>>>>>>> +	xnfree(binding):
>>>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>>  	return err;
>>>>>>>>>>  }
>>>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>>>
>>>>>>>> Ack - now that I see it myself.
>>>>>>>>
>>>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>>>
>>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>>> the descriptor itself is still existing, we rather have to return that
>>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>>> look into this again.
>>>>> It looks to me like a transitory state, we can wait for the sync object
>>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>>> connection -> internal sync objects will be destroyed (to make
>>>> read/write fail). But the fd will remain valid until the local side
>>>> closes it as well.
>>> It looks to me like this is going to complicate things a lot, and will
>>> be a source of regressions. Why can we not have sync objects be
>>> destroyed when the fd is really destroyed and use a status bit of some
>>> kind to signal read/write that the fd was closed by peer?
>> It is way easier and more consistent to unblock reader and writers via
>> destroying the sync object than to signal it and add tests for specific
>> states to detect that. Keep in mind that this pattern is in use even
>> without select support. Diverging from it just to add select awareness
>> to some driver would be a step back.
> 
> Ok. As you wish. But in that case, please provide a unit test case which
> we can run on all architectures to validate your modifications. We will
> not put this test case in xenomai repository but will create a
> repository for test cases later on.

As it requires quite some infrastructure to get there (or do I miss some
preexisting select unit test?), I can just point to RTnet + TCP for now:
run rtnet/examples/xenomai/posix/rttcp-server + client over rtlo and
terminate the client prematurely. This does not happen if you run the
very same test without RTnet loaded (ie. against Linux' TCP).

Just pushed the corresponding fix.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> GIT version control wrote:
>>>>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>>>>  	return 0;
>>>>>>>>>>>  
>>>>>>>>>>>        unlock_and_error:
>>>>>>>>>>> +	xnfree(binding):
>>>>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>>>  	return err;
>>>>>>>>>>>  }
>>>>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>>>>
>>>>>>>>> Ack - now that I see it myself.
>>>>>>>>>
>>>>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>>>>
>>>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>>>> the descriptor itself is still existing, we rather have to return that
>>>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>>>> look into this again.
>>>>>> It looks to me like a transitory state, we can wait for the sync object
>>>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>>>> connection -> internal sync objects will be destroyed (to make
>>>>> read/write fail). But the fd will remain valid until the local side
>>>>> closes it as well.
>>>> It looks to me like this is going to complicate things a lot, and will
>>>> be a source of regressions. Why can we not have sync objects be
>>>> destroyed when the fd is really destroyed and use a status bit of some
>>>> kind to signal read/write that the fd was closed by peer?
>>> It is way easier and more consistent to unblock reader and writers via
>>> destroying the sync object than to signal it and add tests for specific
>>> states to detect that. Keep in mind that this pattern is in use even
>>> without select support. Diverging from it just to add select awareness
>>> to some driver would be a step back.
>> Ok. As you wish. But in that case, please provide a unit test case which
>> we can run on all architectures to validate your modifications. We will
>> not put this test case in xenomai repository but will create a
>> repository for test cases later on.
> 
> As it requires quite some infrastructure to get there (or do I miss some
> preexisting select unit test?), I can just point to RTnet + TCP for now:
> run rtnet/examples/xenomai/posix/rttcp-server + client over rtlo and
> terminate the client prematurely. This does not happen if you run the
> very same test without RTnet loaded (ie. against Linux' TCP).
> 
> Just pushed the corresponding fix.

I really do not understand what you are trying to do. What is the
problem exactly, and how do you fix it? You are reserving 384 more bytes
on the stack. What for?

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>> GIT version control wrote:
>>>>>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>>>>>  	return 0;
>>>>>>>>>>>>  
>>>>>>>>>>>>        unlock_and_error:
>>>>>>>>>>>> +	xnfree(binding):
>>>>>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>>>>  	return err;
>>>>>>>>>>>>  }
>>>>>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>>>>>
>>>>>>>>>> Ack - now that I see it myself.
>>>>>>>>>>
>>>>>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>>>>>
>>>>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>>>>> the descriptor itself is still existing, we rather have to return that
>>>>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>>>>> look into this again.
>>>>>>> It looks to me like a transitory state, we can wait for the sync object
>>>>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>>>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>>>>> connection -> internal sync objects will be destroyed (to make
>>>>>> read/write fail). But the fd will remain valid until the local side
>>>>>> closes it as well.
>>>>> It looks to me like this is going to complicate things a lot, and will
>>>>> be a source of regressions. Why can we not have sync objects be
>>>>> destroyed when the fd is really destroyed and use a status bit of some
>>>>> kind to signal read/write that the fd was closed by peer?
>>>> It is way easier and more consistent to unblock reader and writers via
>>>> destroying the sync object than to signal it and add tests for specific
>>>> states to detect that. Keep in mind that this pattern is in use even
>>>> without select support. Diverging from it just to add select awareness
>>>> to some driver would be a step back.
>>> Ok. As you wish. But in that case, please provide a unit test case which
>>> we can run on all architectures to validate your modifications. We will
>>> not put this test case in xenomai repository but will create a
>>> repository for test cases later on.
>> As it requires quite some infrastructure to get there (or do I miss some
>> preexisting select unit test?), I can just point to RTnet + TCP for now:
>> run rtnet/examples/xenomai/posix/rttcp-server + client over rtlo and
>> terminate the client prematurely. This does not happen if you run the
>> very same test without RTnet loaded (ie. against Linux' TCP).
>>
>> Just pushed the corresponding fix.
> 
> I really do not understand what you are trying to do. What is the
> problem exactly, and how do you fix it? You are reserving 384 more bytes
> on the stack. What for?
> 

"We already return an fd as pending if the corresponding xnsynch object
is delete while waiting on it. Extend select to do the same if the
object is already marked deleted on binding.

This fixes the return code select on half-closed RTnet TCP sockets from
-EIDRM to > 0 (for pending fds)."

HTH.

Regarding the additional stack requirements - yes, not nice. Maybe we
can avoid this by clearing the in_fds in select_bind_all while
processing it unless an fd is marked deleted. Would also avoid passing a
separate fds to it.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>> GIT version control wrote:
>>>>>>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>>>>>>  	return 0;
>>>>>>>>>>>>>  
>>>>>>>>>>>>>        unlock_and_error:
>>>>>>>>>>>>> +	xnfree(binding):
>>>>>>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>>>>>  	return err;
>>>>>>>>>>>>>  }
>>>>>>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>>>>>>
>>>>>>>>>>> Ack - now that I see it myself.
>>>>>>>>>>>
>>>>>>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>>>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>>>>>>
>>>>>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>>>>>> the descriptor itself is still existing, we rather have to return that
>>>>>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>>>>>> look into this again.
>>>>>>>> It looks to me like a transitory state, we can wait for the sync object
>>>>>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>>>>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>>>>>> connection -> internal sync objects will be destroyed (to make
>>>>>>> read/write fail). But the fd will remain valid until the local side
>>>>>>> closes it as well.
>>>>>> It looks to me like this is going to complicate things a lot, and will
>>>>>> be a source of regressions. Why can we not have sync objects be
>>>>>> destroyed when the fd is really destroyed and use a status bit of some
>>>>>> kind to signal read/write that the fd was closed by peer?
>>>>> It is way easier and more consistent to unblock reader and writers via
>>>>> destroying the sync object than to signal it and add tests for specific
>>>>> states to detect that. Keep in mind that this pattern is in use even
>>>>> without select support. Diverging from it just to add select awareness
>>>>> to some driver would be a step back.
>>>> Ok. As you wish. But in that case, please provide a unit test case which
>>>> we can run on all architectures to validate your modifications. We will
>>>> not put this test case in xenomai repository but will create a
>>>> repository for test cases later on.
>>> As it requires quite some infrastructure to get there (or do I miss some
>>> preexisting select unit test?), I can just point to RTnet + TCP for now:
>>> run rtnet/examples/xenomai/posix/rttcp-server + client over rtlo and
>>> terminate the client prematurely. This does not happen if you run the
>>> very same test without RTnet loaded (ie. against Linux' TCP).
>>>
>>> Just pushed the corresponding fix.
>> I really do not understand what you are trying to do. What is the
>> problem exactly, and how do you fix it? You are reserving 384 more bytes
>> on the stack. What for?
>>
> 
> "We already return an fd as pending if the corresponding xnsynch object
> is delete while waiting on it. Extend select to do the same if the
> object is already marked deleted on binding.
> 
> This fixes the return code select on half-closed RTnet TCP sockets from
> -EIDRM to > 0 (for pending fds)."
> 
> HTH.
> 
> Regarding the additional stack requirements - yes, not nice. Maybe we
> can avoid this by clearing the in_fds in select_bind_all while
> processing it unless an fd is marked deleted. Would also avoid passing a
> separate fds to it.

I do not like calling FD_ZERO all the time either. Please take the time
to explain me your implementation (I understood the reason, not the
implementation itself), and to think calmly about the implementation. I
am not going to merge anything before tonight anyway.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>> GIT version control wrote:
>>>>>>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>>>>>>  	return 0;
>>>>>>>>>>>>>  
>>>>>>>>>>>>>        unlock_and_error:
>>>>>>>>>>>>> +	xnfree(binding):
>>>>>>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>>>>>  	return err;
>>>>>>>>>>>>>  }
>>>>>>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>>>>>>
>>>>>>>>>>> Ack - now that I see it myself.
>>>>>>>>>>>
>>>>>>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>>>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>>>>>>
>>>>>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>>>>>> the descriptor itself is still existing, we rather have to return that
>>>>>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>>>>>> look into this again.
>>>>>>>> It looks to me like a transitory state, we can wait for the sync object
>>>>>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>>>>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>>>>>> connection -> internal sync objects will be destroyed (to make
>>>>>>> read/write fail). But the fd will remain valid until the local side
>>>>>>> closes it as well.
>>>>>> It looks to me like this is going to complicate things a lot, and will
>>>>>> be a source of regressions. Why can we not have sync objects be
>>>>>> destroyed when the fd is really destroyed and use a status bit of some
>>>>>> kind to signal read/write that the fd was closed by peer?
>>>>> It is way easier and more consistent to unblock reader and writers via
>>>>> destroying the sync object than to signal it and add tests for specific
>>>>> states to detect that. Keep in mind that this pattern is in use even
>>>>> without select support. Diverging from it just to add select awareness
>>>>> to some driver would be a step back.
>>>> Ok. As you wish. But in that case, please provide a unit test case which
>>>> we can run on all architectures to validate your modifications. We will
>>>> not put this test case in xenomai repository but will create a
>>>> repository for test cases later on.
>>> As it requires quite some infrastructure to get there (or do I miss some
>>> preexisting select unit test?), I can just point to RTnet + TCP for now:
>>> run rtnet/examples/xenomai/posix/rttcp-server + client over rtlo and
>>> terminate the client prematurely. This does not happen if you run the
>>> very same test without RTnet loaded (ie. against Linux' TCP).
>>>
>>> Just pushed the corresponding fix.
>> I really do not understand what you are trying to do. What is the
>> problem exactly, and how do you fix it? You are reserving 384 more bytes
>> on the stack. What for?
>>
> 
> "We already return an fd as pending if the corresponding xnsynch object
> is delete while waiting on it. Extend select to do the same if the
> object is already marked deleted on binding.
> 
> This fixes the return code select on half-closed RTnet TCP sockets from
> -EIDRM to > 0 (for pending fds)."
> 
> HTH.
> 
> Regarding the additional stack requirements - yes, not nice. Maybe we
> can avoid this by clearing the in_fds in select_bind_all while
> processing it unless an fd is marked deleted. Would also avoid passing a
> separate fds to it.

To be more precise, it looks to me like each call to bind_one is
supposed to set the bit of the corresponding fd, so, if bind_one fails
because the fd is in the transitory state (the one which I do not like),
bind_all should simply set this bit to 1. And there does not seem to be
any need for additional parameters, additional space on stack, or
anything else.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>>> GIT version control wrote:
>>>>>>>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>>>>>>>  	return 0;
>>>>>>>>>>>>>>  
>>>>>>>>>>>>>>        unlock_and_error:
>>>>>>>>>>>>>> +	xnfree(binding):
>>>>>>>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>>>>>>  	return err;
>>>>>>>>>>>>>>  }
>>>>>>>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>>>>>>>
>>>>>>>>>>>> Ack - now that I see it myself.
>>>>>>>>>>>>
>>>>>>>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>>>>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>>>>>>>
>>>>>>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>>>>>>> the descriptor itself is still existing, we rather have to return that
>>>>>>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>>>>>>> look into this again.
>>>>>>>>> It looks to me like a transitory state, we can wait for the sync object
>>>>>>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>>>>>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>>>>>>> connection -> internal sync objects will be destroyed (to make
>>>>>>>> read/write fail). But the fd will remain valid until the local side
>>>>>>>> closes it as well.
>>>>>>> It looks to me like this is going to complicate things a lot, and will
>>>>>>> be a source of regressions. Why can we not have sync objects be
>>>>>>> destroyed when the fd is really destroyed and use a status bit of some
>>>>>>> kind to signal read/write that the fd was closed by peer?
>>>>>> It is way easier and more consistent to unblock reader and writers via
>>>>>> destroying the sync object than to signal it and add tests for specific
>>>>>> states to detect that. Keep in mind that this pattern is in use even
>>>>>> without select support. Diverging from it just to add select awareness
>>>>>> to some driver would be a step back.
>>>>> Ok. As you wish. But in that case, please provide a unit test case which
>>>>> we can run on all architectures to validate your modifications. We will
>>>>> not put this test case in xenomai repository but will create a
>>>>> repository for test cases later on.
>>>> As it requires quite some infrastructure to get there (or do I miss some
>>>> preexisting select unit test?), I can just point to RTnet + TCP for now:
>>>> run rtnet/examples/xenomai/posix/rttcp-server + client over rtlo and
>>>> terminate the client prematurely. This does not happen if you run the
>>>> very same test without RTnet loaded (ie. against Linux' TCP).
>>>>
>>>> Just pushed the corresponding fix.
>>> I really do not understand what you are trying to do. What is the
>>> problem exactly, and how do you fix it? You are reserving 384 more bytes
>>> on the stack. What for?
>>>
>> "We already return an fd as pending if the corresponding xnsynch object
>> is delete while waiting on it. Extend select to do the same if the
>> object is already marked deleted on binding.
>>
>> This fixes the return code select on half-closed RTnet TCP sockets from
>> -EIDRM to > 0 (for pending fds)."
>>
>> HTH.
>>
>> Regarding the additional stack requirements - yes, not nice. Maybe we
>> can avoid this by clearing the in_fds in select_bind_all while
>> processing it unless an fd is marked deleted. Would also avoid passing a
>> separate fds to it.
> 
> To be more precise, it looks to me like each call to bind_one is
> supposed to set the bit of the corresponding fd, so, if bind_one fails
> because the fd is in the transitory state (the one which I do not like),
> bind_all should simply set this bit to 1. And there does not seem to be
> any need for additional parameters, additional space on stack, or
> anything else.

Right, the trick is likely to properly maintain the output fds of
bind_all in that not only pending fds are set, but others are cleared -
avoids the third bitmap. Still playing with such an approach.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>>>> GIT version control wrote:
>>>>>>>>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>>>>>>>>  	return 0;
>>>>>>>>>>>>>>>  
>>>>>>>>>>>>>>>        unlock_and_error:
>>>>>>>>>>>>>>> +	xnfree(binding):
>>>>>>>>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>>>>>>>  	return err;
>>>>>>>>>>>>>>>  }
>>>>>>>>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Ack - now that I see it myself.
>>>>>>>>>>>>>
>>>>>>>>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>>>>>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>>>>>>>>
>>>>>>>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>>>>>>>> the descriptor itself is still existing, we rather have to return that
>>>>>>>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>>>>>>>> look into this again.
>>>>>>>>>> It looks to me like a transitory state, we can wait for the sync object
>>>>>>>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>>>>>>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>>>>>>>> connection -> internal sync objects will be destroyed (to make
>>>>>>>>> read/write fail). But the fd will remain valid until the local side
>>>>>>>>> closes it as well.
>>>>>>>> It looks to me like this is going to complicate things a lot, and will
>>>>>>>> be a source of regressions. Why can we not have sync objects be
>>>>>>>> destroyed when the fd is really destroyed and use a status bit of some
>>>>>>>> kind to signal read/write that the fd was closed by peer?
>>>>>>> It is way easier and more consistent to unblock reader and writers via
>>>>>>> destroying the sync object than to signal it and add tests for specific
>>>>>>> states to detect that. Keep in mind that this pattern is in use even
>>>>>>> without select support. Diverging from it just to add select awareness
>>>>>>> to some driver would be a step back.
>>>>>> Ok. As you wish. But in that case, please provide a unit test case which
>>>>>> we can run on all architectures to validate your modifications. We will
>>>>>> not put this test case in xenomai repository but will create a
>>>>>> repository for test cases later on.
>>>>> As it requires quite some infrastructure to get there (or do I miss some
>>>>> preexisting select unit test?), I can just point to RTnet + TCP for now:
>>>>> run rtnet/examples/xenomai/posix/rttcp-server + client over rtlo and
>>>>> terminate the client prematurely. This does not happen if you run the
>>>>> very same test without RTnet loaded (ie. against Linux' TCP).
>>>>>
>>>>> Just pushed the corresponding fix.
>>>> I really do not understand what you are trying to do. What is the
>>>> problem exactly, and how do you fix it? You are reserving 384 more bytes
>>>> on the stack. What for?
>>>>
>>> "We already return an fd as pending if the corresponding xnsynch object
>>> is delete while waiting on it. Extend select to do the same if the
>>> object is already marked deleted on binding.
>>>
>>> This fixes the return code select on half-closed RTnet TCP sockets from
>>> -EIDRM to > 0 (for pending fds)."
>>>
>>> HTH.
>>>
>>> Regarding the additional stack requirements - yes, not nice. Maybe we
>>> can avoid this by clearing the in_fds in select_bind_all while
>>> processing it unless an fd is marked deleted. Would also avoid passing a
>>> separate fds to it.
>> To be more precise, it looks to me like each call to bind_one is
>> supposed to set the bit of the corresponding fd, so, if bind_one fails
>> because the fd is in the transitory state (the one which I do not like),
>> bind_all should simply set this bit to 1. And there does not seem to be
>> any need for additional parameters, additional space on stack, or
>> anything else.
> 
> Right, the trick is likely to properly maintain the output fds of
> bind_all in that not only pending fds are set, but others are cleared -
> avoids the third bitmap. Still playing with such an approach.

xnselect_bind, called by bind_one should take care of setting/clearing
individual bits, at least, it is the way it currently works.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>>>> GIT version control wrote:
>>>>>>>>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>>>>>>>>  	return 0;
>>>>>>>>>>>>>>>  
>>>>>>>>>>>>>>>        unlock_and_error:
>>>>>>>>>>>>>>> +	xnfree(binding):
>>>>>>>>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>>>>>>>  	return err;
>>>>>>>>>>>>>>>  }
>>>>>>>>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Ack - now that I see it myself.
>>>>>>>>>>>>>
>>>>>>>>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>>>>>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>>>>>>>>
>>>>>>>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>>>>>>>> the descriptor itself is still existing, we rather have to return that
>>>>>>>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>>>>>>>> look into this again.
>>>>>>>>>> It looks to me like a transitory state, we can wait for the sync object
>>>>>>>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>>>>>>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>>>>>>>> connection -> internal sync objects will be destroyed (to make
>>>>>>>>> read/write fail). But the fd will remain valid until the local side
>>>>>>>>> closes it as well.
>>>>>>>> It looks to me like this is going to complicate things a lot, and will
>>>>>>>> be a source of regressions. Why can we not have sync objects be
>>>>>>>> destroyed when the fd is really destroyed and use a status bit of some
>>>>>>>> kind to signal read/write that the fd was closed by peer?
>>>>>>> It is way easier and more consistent to unblock reader and writers via
>>>>>>> destroying the sync object than to signal it and add tests for specific
>>>>>>> states to detect that. Keep in mind that this pattern is in use even
>>>>>>> without select support. Diverging from it just to add select awareness
>>>>>>> to some driver would be a step back.
>>>>>> Ok. As you wish. But in that case, please provide a unit test case which
>>>>>> we can run on all architectures to validate your modifications. We will
>>>>>> not put this test case in xenomai repository but will create a
>>>>>> repository for test cases later on.
>>>>> As it requires quite some infrastructure to get there (or do I miss some
>>>>> preexisting select unit test?), I can just point to RTnet + TCP for now:
>>>>> run rtnet/examples/xenomai/posix/rttcp-server + client over rtlo and
>>>>> terminate the client prematurely. This does not happen if you run the
>>>>> very same test without RTnet loaded (ie. against Linux' TCP).
>>>>>
>>>>> Just pushed the corresponding fix.
>>>> I really do not understand what you are trying to do. What is the
>>>> problem exactly, and how do you fix it? You are reserving 384 more bytes
>>>> on the stack. What for?
>>>>
>>> "We already return an fd as pending if the corresponding xnsynch object
>>> is delete while waiting on it. Extend select to do the same if the
>>> object is already marked deleted on binding.
>>>
>>> This fixes the return code select on half-closed RTnet TCP sockets from
>>> -EIDRM to > 0 (for pending fds)."
>>>
>>> HTH.
>>>
>>> Regarding the additional stack requirements - yes, not nice. Maybe we
>>> can avoid this by clearing the in_fds in select_bind_all while
>>> processing it unless an fd is marked deleted. Would also avoid passing a
>>> separate fds to it.
>> To be more precise, it looks to me like each call to bind_one is
>> supposed to set the bit of the corresponding fd, so, if bind_one fails
>> because the fd is in the transitory state (the one which I do not like),
>> bind_all should simply set this bit to 1. And there does not seem to be
>> any need for additional parameters, additional space on stack, or
>> anything else.
> 
> Right, the trick is likely to properly maintain the output fds of
> bind_all in that not only pending fds are set, but others are cleared -
> avoids the third bitmap. Still playing with such an approach.

Another precision, xnselect_bind accesses directly the "pending" set of
the xnselector structure, so, you have the right to do that for the fds
which failed binding but which you want to mark as pending.

> 
> Jan
> 


-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> To be more precise, it looks to me like each call to bind_one is
>>> supposed to set the bit of the corresponding fd, so, if bind_one fails
>>> because the fd is in the transitory state (the one which I do not like),
>>> bind_all should simply set this bit to 1. And there does not seem to be
>>> any need for additional parameters, additional space on stack, or
>>> anything else.
>> Right, the trick is likely to properly maintain the output fds of
>> bind_all in that not only pending fds are set, but others are cleared -
>> avoids the third bitmap. Still playing with such an approach.
> 
> Another precision, xnselect_bind accesses directly the "pending" set of
> the xnselector structure, so, you have the right to do that for the fds
> which failed binding but which you want to mark as pending.

Right, /me was blind, the issue can trivially be fixed inside RTDM by
binding as pending even if the object is marked deleted. I'm now just
cleaning up some other RTDM bits at this chance, pull request will follow.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Right, the trick is likely to properly maintain the output fds of
> bind_all in that not only pending fds are set, but others are cleared -
> avoids the third bitmap. Still playing with such an approach.

What we are trying to do, in a nutshell, is to create a notion of
stateless binding between a file descriptor and an xnselector. If we
really want to do this, we will have to extend the xnselect interface.
But this looks really wrong to me.

It seems possible to keep the bindings around as long as the file
descriptor exists. The binding is here for this reason, to express the
link between the file descriptor and the thread which runs select, and
its lifecycle should be the same as the file descriptor. And if you want
to signal that such a file descriptor is ready for reading/writing, even
if it is closed, you should be using xnselect_signal. The interface is
there, it is meant for that.

So, the more I think about it, the more I think that we are going in the
wrong direction.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : RTDM+POSIX: Avoid leaking binding objects on errors

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> GIT version control wrote:
>>>>>>>>> diff --git a/ksrc/skins/posix/mq.c b/ksrc/skins/posix/mq.c
>>>>>>>>> index 11f47c0..a896031 100644
>>>>>>>>> --- a/ksrc/skins/posix/mq.c
>>>>>>>>> +++ b/ksrc/skins/posix/mq.c
>>>>>>>>> @@ -1283,6 +1283,7 @@ int pse51_mq_select_bind(mqd_t fd, struct xnselector *selector,
>>>>>>>>>  	return 0;
>>>>>>>>>  
>>>>>>>>>        unlock_and_error:
>>>>>>>>> +	xnfree(binding):
>>>>>>>>>  	xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>  	return err;
>>>>>>>>>  }
>>>>>>>> Ok. Will pull. But I really need to fix that.
>>>>>>>>
>>>>>>> Ack - now that I see it myself.
>>>>>>>
>>>>>> I fixed this in my branch and added another patch to transform EIDRM
>>>>>> into EBADF when selecting a (half-)deleted RTDM device. Please merge.
>>>>>>
>>>>> Wait! When the sync object behind some file descriptor is deleted but
>>>>> the descriptor itself is still existing, we rather have to return that
>>>>> fd signaled from select() instead of letting the call fail. I beed to
>>>>> look into this again.
>>>> It looks to me like a transitory state, we can wait for the sync object
>>>> to be deleted to have the fd destructor signaled. It should not be long.
>>> That's not an issue of waiting for this. See e.g. TCP: peer closes
>>> connection -> internal sync objects will be destroyed (to make
>>> read/write fail). But the fd will remain valid until the local side
>>> closes it as well.
>> It looks to me like this is going to complicate things a lot, and will
>> be a source of regressions. Why can we not have sync objects be
>> destroyed when the fd is really destroyed and use a status bit of some
>> kind to signal read/write that the fd was closed by peer?
> 
> It is way easier and more consistent to unblock reader and writers via
> destroying the sync object than to signal it and add tests for specific
> states to detect that.

>From my point of view, having an object with partially destroyed parts
is asking for trouble.

-- 
					    Gilles.