Re: Kevent POSIX timers support.
Hello. Please _NEVER_ drop Cc: list, since not everyone can be subscribed to linux-kernel@, fortunately I'm not for example. On Wed, Dec 13, 2006 at 06:51:47PM +0530, Tushar Adeshara ([EMAIL PROTECTED]) wrote: > I think these four lines are not required. Irrespective of return > value of kevent_user_add_ukevent(), we are going to release file, and > return err. > >+ if (err) > >+ goto err_out_fput; > >+ > >+ fput(file); > >+ > >+ return 0; > > > >+ > >+err_out_fput: > >+ fput(file); > >+err_out: > >+ return err; > >+} > >+ I put them to always know where error path is and where it is not, so it could be easier to put error statistic or debug. It can be removed, but imho it reduces readability. -- Evgeniy Polyakov - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
On 11/23/06, Evgeniy Polyakov <[EMAIL PROTECTED]> wrote:
On Wed, Nov 22, 2006 at 01:44:16PM +0300, Evgeniy Polyakov ([EMAIL PROTECTED])
wrote:
+static int posix_kevent_init_timer(struct k_itimer *tmr, int fd)
+{
+ struct ukevent uk;
+ struct file *file;
+ struct kevent_user *u;
+ int err;
+
+ file = fget(fd);
+ if (!file) {
+ err = -EBADF;
+ goto err_out;
+ }
+
+ if (file->f_op != _user_fops) {
+ err = -EINVAL;
+ goto err_out_fput;
+ }
+
+ u = file->private_data;
+
+ memset(, 0, sizeof(struct ukevent));
+
+ uk.type = KEVENT_POSIX_TIMER;
+ uk.id.raw_u64 = (unsigned long)(tmr); /* Just cast to something unique
*/
+ uk.ptr = tmr;
+
+ tmr->it_sigev_value.sival_ptr = file;
+
+ err = kevent_user_add_ukevent(, u);
I think these four lines are not required. Irrespective of return
value of kevent_user_add_ukevent(), we are going to release file, and
return err.
+ if (err)
+ goto err_out_fput;
+
+ fput(file);
+
+ return 0;
+
+err_out_fput:
+ fput(file);
+err_out:
+ return err;
+}
+
--
Regards,
Tushar
It's not a problem, it's an opportunity for improvement. Lets improve.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
Hello. Please _NEVER_ drop Cc: list, since not everyone can be subscribed to linux-kernel@, fortunately I'm not for example. On Wed, Dec 13, 2006 at 06:51:47PM +0530, Tushar Adeshara ([EMAIL PROTECTED]) wrote: I think these four lines are not required. Irrespective of return value of kevent_user_add_ukevent(), we are going to release file, and return err. + if (err) + goto err_out_fput; + + fput(file); + + return 0; + +err_out_fput: + fput(file); +err_out: + return err; +} + I put them to always know where error path is and where it is not, so it could be easier to put error statistic or debug. It can be removed, but imho it reduces readability. -- Evgeniy Polyakov - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
On Mon, Dec 11, 2006 at 05:36:44PM -0800, David Miller ([EMAIL PROTECTED]) wrote: > From: Evgeniy Polyakov <[EMAIL PROTECTED]> > Date: Tue, 28 Nov 2006 22:22:36 +0300 > > > And, btw, last time I checked, aligned_u64 was not exported to > > userspace. > > It is in linux/types.h and not protected by __KERNEL__ ifdefs. > Perhaps you mean something else? It looks like I checked wrong #ifdef __KERNEL__/#endif pair. It is there indeed. -- Evgeniy Polyakov - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
From: Evgeniy Polyakov <[EMAIL PROTECTED]> Date: Tue, 28 Nov 2006 22:22:36 +0300 > And, btw, last time I checked, aligned_u64 was not exported to > userspace. It is in linux/types.h and not protected by __KERNEL__ ifdefs. Perhaps you mean something else? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
From: Evgeniy Polyakov [EMAIL PROTECTED] Date: Tue, 28 Nov 2006 22:22:36 +0300 And, btw, last time I checked, aligned_u64 was not exported to userspace. It is in linux/types.h and not protected by __KERNEL__ ifdefs. Perhaps you mean something else? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
On Mon, Dec 11, 2006 at 05:36:44PM -0800, David Miller ([EMAIL PROTECTED]) wrote: From: Evgeniy Polyakov [EMAIL PROTECTED] Date: Tue, 28 Nov 2006 22:22:36 +0300 And, btw, last time I checked, aligned_u64 was not exported to userspace. It is in linux/types.h and not protected by __KERNEL__ ifdefs. Perhaps you mean something else? It looks like I checked wrong #ifdef __KERNEL__/#endif pair. It is there indeed. -- Evgeniy Polyakov - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
On Tue, Nov 28, 2006 at 11:13:00AM -0800, David Miller ([EMAIL PROTECTED]) wrote: > From: Evgeniy Polyakov <[EMAIL PROTECTED]> > Date: Tue, 28 Nov 2006 12:16:02 +0300 > > > Although ukevent has pointer embedded, it is unioned with u64, so there > > should be no problems until 128 bit arch appeared, which likely will not > > happen soon. There is also unused in kevent posix timers patch > > 'u32 ret_val[2]' field, which can store segval's value too. > > > > But the fact that ukevent does not and will not in any way have variable > > size is absolutely. > > I believe that in order to be %100 safe you will need to use the > special aligned_u64 type, as that takes care of a crucial difference > between x86 and x86_64 API, namely that u64 needs 8-byte alignment on > x86_64 but not on x86. > > You probably know this already :-) Yep :) So I put it at the end, where structure is already correctly aligned, so there is no need for special alignment. And, btw, last time I checked, aligned_u64 was not exported to userspace. -- Evgeniy Polyakov - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
From: Evgeniy Polyakov <[EMAIL PROTECTED]> Date: Tue, 28 Nov 2006 12:16:02 +0300 > Although ukevent has pointer embedded, it is unioned with u64, so there > should be no problems until 128 bit arch appeared, which likely will not > happen soon. There is also unused in kevent posix timers patch > 'u32 ret_val[2]' field, which can store segval's value too. > > But the fact that ukevent does not and will not in any way have variable > size is absolutely. I believe that in order to be %100 safe you will need to use the special aligned_u64 type, as that takes care of a crucial difference between x86 and x86_64 API, namely that u64 needs 8-byte alignment on x86_64 but not on x86. You probably know this already :-) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
On Mon, Nov 27, 2006 at 10:20:50AM -0800, Ulrich Drepper ([EMAIL PROTECTED]) wrote: > sigev_value is a union and the largest element is a pointer. So, > transporting the pointer value is sufficient and it should be passed up > to the user in the ptr member of struct ukevent. That is where I've put it in current version. -- Evgeniy Polyakov - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
On Mon, Nov 27, 2006 at 10:49:55AM -0800, David Miller ([EMAIL PROTECTED]) wrote: > From: Ulrich Drepper <[EMAIL PROTECTED]> > Date: Mon, 27 Nov 2006 10:36:06 -0800 > > > David Miller wrote: > > > Now we'll have to have a compat layer for 32-bit/64-bit environments > > > thanks to POSIX timers, which is rediculious. > > > > We already have compat_sys_timer_create. It should be sufficient just > > to add the conversion (if anything new is needed) there. The pointer > > value can be passed to userland in one or two int fields, I don't really > > care. When reporting the event to the user code we cannot just point > > into the ring buffer anyway. So while copying the data we can rewrite > > it if necessary. I see no need to complicate the code more than it > > already is. > > Ok, as long as that thing doesn't end up in the ring buffer entry > data structure, that's where the real troubles would be. Although ukevent has pointer embedded, it is unioned with u64, so there should be no problems until 128 bit arch appeared, which likely will not happen soon. There is also unused in kevent posix timers patch 'u32 ret_val[2]' field, which can store segval's value too. But the fact that ukevent does not and will not in any way have variable size is absolutely. -- Evgeniy Polyakov - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
On Mon, Nov 27, 2006 at 10:49:55AM -0800, David Miller ([EMAIL PROTECTED]) wrote: From: Ulrich Drepper [EMAIL PROTECTED] Date: Mon, 27 Nov 2006 10:36:06 -0800 David Miller wrote: Now we'll have to have a compat layer for 32-bit/64-bit environments thanks to POSIX timers, which is rediculious. We already have compat_sys_timer_create. It should be sufficient just to add the conversion (if anything new is needed) there. The pointer value can be passed to userland in one or two int fields, I don't really care. When reporting the event to the user code we cannot just point into the ring buffer anyway. So while copying the data we can rewrite it if necessary. I see no need to complicate the code more than it already is. Ok, as long as that thing doesn't end up in the ring buffer entry data structure, that's where the real troubles would be. Although ukevent has pointer embedded, it is unioned with u64, so there should be no problems until 128 bit arch appeared, which likely will not happen soon. There is also unused in kevent posix timers patch 'u32 ret_val[2]' field, which can store segval's value too. But the fact that ukevent does not and will not in any way have variable size is absolutely. -- Evgeniy Polyakov - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
On Mon, Nov 27, 2006 at 10:20:50AM -0800, Ulrich Drepper ([EMAIL PROTECTED]) wrote: sigev_value is a union and the largest element is a pointer. So, transporting the pointer value is sufficient and it should be passed up to the user in the ptr member of struct ukevent. That is where I've put it in current version. -- Evgeniy Polyakov - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
From: Evgeniy Polyakov [EMAIL PROTECTED] Date: Tue, 28 Nov 2006 12:16:02 +0300 Although ukevent has pointer embedded, it is unioned with u64, so there should be no problems until 128 bit arch appeared, which likely will not happen soon. There is also unused in kevent posix timers patch 'u32 ret_val[2]' field, which can store segval's value too. But the fact that ukevent does not and will not in any way have variable size is absolutely. I believe that in order to be %100 safe you will need to use the special aligned_u64 type, as that takes care of a crucial difference between x86 and x86_64 API, namely that u64 needs 8-byte alignment on x86_64 but not on x86. You probably know this already :-) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
On Tue, Nov 28, 2006 at 11:13:00AM -0800, David Miller ([EMAIL PROTECTED]) wrote: From: Evgeniy Polyakov [EMAIL PROTECTED] Date: Tue, 28 Nov 2006 12:16:02 +0300 Although ukevent has pointer embedded, it is unioned with u64, so there should be no problems until 128 bit arch appeared, which likely will not happen soon. There is also unused in kevent posix timers patch 'u32 ret_val[2]' field, which can store segval's value too. But the fact that ukevent does not and will not in any way have variable size is absolutely. I believe that in order to be %100 safe you will need to use the special aligned_u64 type, as that takes care of a crucial difference between x86 and x86_64 API, namely that u64 needs 8-byte alignment on x86_64 but not on x86. You probably know this already :-) Yep :) So I put it at the end, where structure is already correctly aligned, so there is no need for special alignment. And, btw, last time I checked, aligned_u64 was not exported to userspace. -- Evgeniy Polyakov - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
From: Ulrich Drepper <[EMAIL PROTECTED]> Date: Mon, 27 Nov 2006 10:36:06 -0800 > David Miller wrote: > > Now we'll have to have a compat layer for 32-bit/64-bit environments > > thanks to POSIX timers, which is rediculious. > > We already have compat_sys_timer_create. It should be sufficient just > to add the conversion (if anything new is needed) there. The pointer > value can be passed to userland in one or two int fields, I don't really > care. When reporting the event to the user code we cannot just point > into the ring buffer anyway. So while copying the data we can rewrite > it if necessary. I see no need to complicate the code more than it > already is. Ok, as long as that thing doesn't end up in the ring buffer entry data structure, that's where the real troubles would be. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
David Miller wrote: Now we'll have to have a compat layer for 32-bit/64-bit environments thanks to POSIX timers, which is rediculious. We already have compat_sys_timer_create. It should be sufficient just to add the conversion (if anything new is needed) there. The pointer value can be passed to userland in one or two int fields, I don't really care. When reporting the event to the user code we cannot just point into the ring buffer anyway. So while copying the data we can rewrite it if necessary. I see no need to complicate the code more than it already is. -- ➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
From: Ulrich Drepper <[EMAIL PROTECTED]> Date: Mon, 27 Nov 2006 10:20:50 -0800 > Evgeniy Polyakov wrote: > >> We need to pass the data in the sigev_value meember of the struct > >> sigevent structure passed to timer_create to the caller. I don't see it > >> being done here nor when the timer is created. Do I miss something? > >> The sigev_value value should be stored in the user/ptr member of struct > >> ukevent. > > > > sigev_value was stored in k_itimer structure, I just do not know where > > to put it in the ukevent provided to userspace - it can be placed in > > pointer value if you like. > > sigev_value is a union and the largest element is a pointer. So, > transporting the pointer value is sufficient and it should be passed up > to the user in the ptr member of struct ukevent. Now we'll have to have a compat layer for 32-bit/64-bit environments thanks to POSIX timers, which is rediculious. This is exactly the kind of thing I was hoping we could avoid when designing these data structures. No pointers, no non-fixed sized types, only types which are identically sized and aligned between 32-bit and 64-bit environments. It's OK to have these problems for things designed a long time ago before 32-bit/64-bit compat issues existed, but for new stuff no way. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
Evgeniy Polyakov wrote: We need to pass the data in the sigev_value meember of the struct sigevent structure passed to timer_create to the caller. I don't see it being done here nor when the timer is created. Do I miss something? The sigev_value value should be stored in the user/ptr member of struct ukevent. sigev_value was stored in k_itimer structure, I just do not know where to put it in the ukevent provided to userspace - it can be placed in pointer value if you like. sigev_value is a union and the largest element is a pointer. So, transporting the pointer value is sufficient and it should be passed up to the user in the ptr member of struct ukevent. -- ➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
Evgeniy Polyakov wrote: We need to pass the data in the sigev_value meember of the struct sigevent structure passed to timer_create to the caller. I don't see it being done here nor when the timer is created. Do I miss something? The sigev_value value should be stored in the user/ptr member of struct ukevent. sigev_value was stored in k_itimer structure, I just do not know where to put it in the ukevent provided to userspace - it can be placed in pointer value if you like. sigev_value is a union and the largest element is a pointer. So, transporting the pointer value is sufficient and it should be passed up to the user in the ptr member of struct ukevent. -- ➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
From: Ulrich Drepper [EMAIL PROTECTED] Date: Mon, 27 Nov 2006 10:20:50 -0800 Evgeniy Polyakov wrote: We need to pass the data in the sigev_value meember of the struct sigevent structure passed to timer_create to the caller. I don't see it being done here nor when the timer is created. Do I miss something? The sigev_value value should be stored in the user/ptr member of struct ukevent. sigev_value was stored in k_itimer structure, I just do not know where to put it in the ukevent provided to userspace - it can be placed in pointer value if you like. sigev_value is a union and the largest element is a pointer. So, transporting the pointer value is sufficient and it should be passed up to the user in the ptr member of struct ukevent. Now we'll have to have a compat layer for 32-bit/64-bit environments thanks to POSIX timers, which is rediculious. This is exactly the kind of thing I was hoping we could avoid when designing these data structures. No pointers, no non-fixed sized types, only types which are identically sized and aligned between 32-bit and 64-bit environments. It's OK to have these problems for things designed a long time ago before 32-bit/64-bit compat issues existed, but for new stuff no way. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
David Miller wrote: Now we'll have to have a compat layer for 32-bit/64-bit environments thanks to POSIX timers, which is rediculious. We already have compat_sys_timer_create. It should be sufficient just to add the conversion (if anything new is needed) there. The pointer value can be passed to userland in one or two int fields, I don't really care. When reporting the event to the user code we cannot just point into the ring buffer anyway. So while copying the data we can rewrite it if necessary. I see no need to complicate the code more than it already is. -- ➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kevent POSIX timers support.
From: Ulrich Drepper [EMAIL PROTECTED] Date: Mon, 27 Nov 2006 10:36:06 -0800 David Miller wrote: Now we'll have to have a compat layer for 32-bit/64-bit environments thanks to POSIX timers, which is rediculious. We already have compat_sys_timer_create. It should be sufficient just to add the conversion (if anything new is needed) there. The pointer value can be passed to userland in one or two int fields, I don't really care. When reporting the event to the user code we cannot just point into the ring buffer anyway. So while copying the data we can rewrite it if necessary. I see no need to complicate the code more than it already is. Ok, as long as that thing doesn't end up in the ring buffer entry data structure, that's where the real troubles would be. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
