Hello All
I analyze kernel vanila 2.6.4 and found one possible bug in
__kill_pg_info function.
for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
err = group_send_sig_info(sig, info, p);
if (retval)
retval = err;
}
but I think if (retval) is incorrect check. possible this cycle must be
for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
err = group_send_sig_info(sig, info, p);
if (ret) {
retval = err;
break;
}
}
because in original variant me assign to retval only first value from
ret and other be ignored if this value be 0.
--
Alex Lyashkov <[email protected]>
PSoft
Alex Lyashkov <[email protected]> wrote:
>
> Hello All
>
> I analyze kernel vanila 2.6.4 and found one possible bug in
> __kill_pg_info function.
>
> for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> err = group_send_sig_info(sig, info, p);
> if (retval)
> retval = err;
> }
> but I think if (retval) is incorrect check. possible this cycle must be
> for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> err = group_send_sig_info(sig, info, p);
> if (ret) {
> retval = err;
> break;
> }
> }
> because in original variant me assign to retval only first value from
> ret and other be ignored if this value be 0.
>
No, the code's OK, albeit undesirably obscure. It will return -ESRCH if
none of the tasks had a matching pgrp and will return the result of the
final non-zero-returning group_send_sig_info() if one or more of the
group_send_sig_info() calls failed, and will return zero if all of the
group_send_sig_info() calls returned zero.
Thanks for checking though..
? ???, 14.03.2004, ? 03:18, Andrew Morton ?????:
> Alex Lyashkov <[email protected]> wrote:
> >
> > Hello All
> >
> > I analyze kernel vanila 2.6.4 and found one possible bug in
> > __kill_pg_info function.
> >
> > for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> > err = group_send_sig_info(sig, info, p);
> > if (retval)
> > retval = err;
> > }
> > but I think if (retval) is incorrect check. possible this cycle must be
> > for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> > err = group_send_sig_info(sig, info, p);
> > if (ret) {
> > retval = err;
> > break;
> > }
> > }
> > because in original variant me assign to retval only first value from
> > ret and other be ignored if this value be 0.
> >
>
> No, the code's OK, albeit undesirably obscure. It will return -ESRCH if
> none of the tasks had a matching pgrp and will return the result of the
> final non-zero-returning group_send_sig_info() if one or more of the
> group_send_sig_info() calls failed, and will return zero if all of the
> group_send_sig_info() calls returned zero.
>
> Thanks for checking though..
No. it can`t return final non-zero-returning group_send_sig_info() if
first call group_send_sig_info return 0.
Example me have 3 groups it will return
1 - zero
2 - nonzero (example -1)
3 - nonzero (example -2)
original code will return zero.
but you say it will return last non zero - "-2" in example.
(do only first assignment after it retval is zero and no assignments
does.)
For her logic me can write.
=====
int retval = 0;
int err = -1;
for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
err = group_send_sig_info(sig, info, p);
if( err )
retval = err;
}
return err==-1 ? -ESRCH : retval;
===
If me not enter to this cycle - retval = -ESRCH, and last non zero err
if cycle is worked or zero if all group_send_sig_info() return zero.
--
Alex Lyashkov <[email protected]>
PSoft
Alex Lyashkov <[email protected]> wrote:
>
> > Thanks for checking though..
> No. it can`t return final non-zero-returning group_send_sig_info() if
> first call group_send_sig_info return 0.
you're right. How about the nice and simple version?
int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp)
{
struct task_struct *p;
struct list_head *l;
struct pid *pid;
int retval;
int found;
if (pgrp <= 0)
return -EINVAL;
found = 0;
retval = 0;
for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
int err;
found = 1;
err = group_send_sig_info(sig, info, p);
if (!retval)
retval = err;
}
return found ? retval : -ESRCH;
}
? ???, 14.03.2004, ? 07:00, Andrew Morton ?????:
> Alex Lyashkov <[email protected]> wrote:
> >
> > > Thanks for checking though..
> > No. it can`t return final non-zero-returning group_send_sig_info() if
> > first call group_send_sig_info return 0.
>
> you're right. How about the nice and simple version?
>
> int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp)
> {
> struct task_struct *p;
> struct list_head *l;
> struct pid *pid;
> int retval;
> int found;
>
> if (pgrp <= 0)
> return -EINVAL;
>
> found = 0;
> retval = 0;
> for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> int err;
>
> found = 1;
> err = group_send_sig_info(sig, info, p);
> if (!retval)
> retval = err;
> }
> return found ? retval : -ESRCH;
> }
not. it error. At this code you save first non zero value err but other
been ignored.
--
Alex Lyashkov <[email protected]>
PSoft
Alex Lyashkov <[email protected]> wrote:
>
> > int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp)
> > {
> > struct task_struct *p;
> > struct list_head *l;
> > struct pid *pid;
> > int retval;
> > int found;
> >
> > if (pgrp <= 0)
> > return -EINVAL;
> >
> > found = 0;
> > retval = 0;
> > for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> > int err;
> >
> > found = 1;
> > err = group_send_sig_info(sig, info, p);
> > if (!retval)
> > retval = err;
> > }
> > return found ? retval : -ESRCH;
> > }
> not. it error. At this code you save first non zero value err but other
> been ignored.
Well we can only return one error code. Or are you suggesting that we
should terminate the loop early on error? If so, why?
? ???, 14.03.2004, ? 07:47, Andrew Morton ?????:
> Alex Lyashkov <[email protected]> wrote:
> >
> > > int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp)
> > > {
> > > struct task_struct *p;
> > > struct list_head *l;
> > > struct pid *pid;
> > > int retval;
> > > int found;
> > >
> > > if (pgrp <= 0)
> > > return -EINVAL;
> > >
> > > found = 0;
> > > retval = 0;
> > > for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> > > int err;
> > >
> > > found = 1;
> > > err = group_send_sig_info(sig, info, p);
> > > if (!retval)
> > > retval = err;
> > > }
> > > return found ? retval : -ESRCH;
> > > }
> > not. it error. At this code you save first non zero value err but other
> > been ignored.
>
> Well we can only return one error code. Or are you suggesting that we
> should terminate the loop early on error? If so, why?
You say me can return _last_ error core. but this function return
_first_.
I write second variant where not terminate loop and save _last_ error
code (i was sending in previous mail). but if you have i write full
function:
====
int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp)
{
struct task_struct *p;
struct list_head *l;
struct pid *pid;
int retval = 0;
int err = -1;
if (pgrp <= 0)
return -EINVAL;
for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
err = group_send_sig_info(sig, info, p);
if( err )
retval = err;
}
return err==-1 ? -ESRCH : retval;
}
===
what you think about its code ?
--
Alex Lyashkov <[email protected]>
PSoft
Alex Lyashkov <[email protected]> wrote:
>
> > Well we can only return one error code. Or are you suggesting that we
> > should terminate the loop early on error? If so, why?
> You say me can return _last_ error core. but this function return
> _first_.
It doesn't matter, really. Other parts of the kernel will generally return
the first-encountered error because at times it _does_ matter. But here it
doesn't.
? ???, 14.03.2004, ? 08:09, Andrew Morton ?????:
> Alex Lyashkov <[email protected]> wrote:
> >
> > > Well we can only return one error code. Or are you suggesting that we
> > > should terminate the loop early on error? If so, why?
> > You say me can return _last_ error core. but this function return
> > _first_.
>
> It doesn't matter, really. Other parts of the kernel will generally return
> the first-encountered error because at times it _does_ matter. But here it
> doesn't.
okey. second question.
a really need extra variable instead change conditions in return ?
--
Alex Lyashkov <[email protected]>
PSoft
Alex Lyashkov <[email protected]> wrote:
>
> __ ______, 14.03.2004, __ 08:09, Andrew Morton __________:
> > Alex Lyashkov <[email protected]> wrote:
> > >
> > > > Well we can only return one error code. Or are you suggesting that we
> > > > should terminate the loop early on error? If so, why?
> > > You say me can return _last_ error core. but this function return
> > > _first_.
> >
> > It doesn't matter, really. Other parts of the kernel will generally return
> > the first-encountered error because at times it _does_ matter. But here it
> > doesn't.
> okey. second question.
> a really need extra variable instead change conditions in return ?
I think it's clearer that way, don't you?