[PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS - Kernel

This is a discussion on [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS - Kernel ; cinergyT2, remove bad usage of ERESTARTSYS test of cinergyt2->disconnect_pending doesn't ensure pending signal and so ERESTARTSYS would reach userspace, which is not permitted. Change it to EAGAIN Signed-off-by: Jiri Slaby --- commit b227f5517ddd99f581a9d241c8ba9c50c50fbc3e tree f469eea4a298db2d4fe27313e48901d74211b2ca parent 235cf594bc65128250632a642f3e9d7e4df4975e author Jiri Slaby ...

+ Reply to Thread
Results 1 to 11 of 11

Thread: [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

  1. [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

    cinergyT2, remove bad usage of ERESTARTSYS

    test of cinergyt2->disconnect_pending doesn't ensure pending signal and so
    ERESTARTSYS would reach userspace, which is not permitted. Change it to
    EAGAIN

    Signed-off-by: Jiri Slaby

    ---
    commit b227f5517ddd99f581a9d241c8ba9c50c50fbc3e
    tree f469eea4a298db2d4fe27313e48901d74211b2ca
    parent 235cf594bc65128250632a642f3e9d7e4df4975e
    author Jiri Slaby Mon, 08 Oct 2007 14:24:32 +0200
    committer Jiri Slaby Mon, 08 Oct 2007 14:24:32 +0200

    drivers/media/dvb/cinergyT2/cinergyT2.c | 38 ++++++++++++++++++++-----------
    1 files changed, 25 insertions(+), 13 deletions(-)

    diff --git a/drivers/media/dvb/cinergyT2/cinergyT2.c b/drivers/media/dvb/cinergyT2/cinergyT2.c
    index 154a7ce..a1f6ebd 100644
    --- a/drivers/media/dvb/cinergyT2/cinergyT2.c
    +++ b/drivers/media/dvb/cinergyT2/cinergyT2.c
    @@ -345,7 +345,9 @@ static int cinergyt2_start_feed(struct dvb_demux_feed *dvbdmxfeed)
    struct dvb_demux *demux = dvbdmxfeed->demux;
    struct cinergyt2 *cinergyt2 = demux->priv;

    - if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
    + if (cinergyt2->disconnect_pending)
    + return -EAGAIN;
    + if (mutex_lock_interruptible(&cinergyt2->sem))
    return -ERESTARTSYS;

    if (cinergyt2->streaming == 0)
    @@ -361,7 +363,9 @@ static int cinergyt2_stop_feed(struct dvb_demux_feed *dvbdmxfeed)
    struct dvb_demux *demux = dvbdmxfeed->demux;
    struct cinergyt2 *cinergyt2 = demux->priv;

    - if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
    + if (cinergyt2->disconnect_pending)
    + return -EAGAIN;
    + if (mutex_lock_interruptible(&cinergyt2->sem))
    return -ERESTARTSYS;

    if (--cinergyt2->streaming == 0)
    @@ -481,12 +485,14 @@ static int cinergyt2_open (struct inode *inode, struct file *file)
    {
    struct dvb_device *dvbdev = file->private_data;
    struct cinergyt2 *cinergyt2 = dvbdev->priv;
    - int err = -ERESTARTSYS;
    + int err = -EAGAIN;

    - if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->wq_sem))
    + if (cinergyt2->disconnect_pending)
    + goto out;
    + if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
    goto out;

    - if (mutex_lock_interruptible(&cinergyt2->sem))
    + if ((err = mutex_lock_interruptible(&cinergyt2->sem)))
    goto out_unlock1;

    if ((err = dvb_generic_open(inode, file)))
    @@ -550,7 +556,9 @@ static unsigned int cinergyt2_poll (struct file *file, struct poll_table_struct
    struct cinergyt2 *cinergyt2 = dvbdev->priv;
    unsigned int mask = 0;

    - if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
    + if (cinergyt2->disconnect_pending)
    + return -EAGAIN;
    + if (mutex_lock_interruptible(&cinergyt2->sem))
    return -ERESTARTSYS;

    poll_wait(file, &cinergyt2->poll_wq, wait);
    @@ -625,7 +633,9 @@ static int cinergyt2_ioctl (struct inode *inode, struct file *file,
    if (copy_from_user(&p, (void __user*) arg, sizeof(p)))
    return -EFAULT;

    - if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
    + if (cinergyt2->disconnect_pending)
    + return -EAGAIN;
    + if (mutex_lock_interruptible(&cinergyt2->sem))
    return -ERESTARTSYS;

    param->cmd = CINERGYT2_EP1_SET_TUNER_PARAMETERS;
    @@ -996,7 +1006,9 @@ static int cinergyt2_suspend (struct usb_interface *intf, pm_message_t state)
    {
    struct cinergyt2 *cinergyt2 = usb_get_intfdata (intf);

    - if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->wq_sem))
    + if (cinergyt2->disconnect_pending)
    + return -EAGAIN;
    + if (mutex_lock_interruptible(&cinergyt2->wq_sem))
    return -ERESTARTSYS;

    cinergyt2_suspend_rc(cinergyt2);
    @@ -1017,16 +1029,16 @@ static int cinergyt2_resume (struct usb_interface *intf)
    {
    struct cinergyt2 *cinergyt2 = usb_get_intfdata (intf);
    struct dvbt_set_parameters_msg *param = &cinergyt2->param;
    - int err = -ERESTARTSYS;
    + int err = -EAGAIN;

    - if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->wq_sem))
    + if (cinergyt2->disconnect_pending)
    + goto out;
    + if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
    goto out;

    - if (mutex_lock_interruptible(&cinergyt2->sem))
    + if ((err = mutex_lock_interruptible(&cinergyt2->sem)))
    goto out_unlock1;

    - err = 0;
    -
    if (!cinergyt2->sleeping) {
    cinergyt2_sleep(cinergyt2, 0);
    cinergyt2_command(cinergyt2, (char *) param, sizeof(*param), NULL, 0);
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

    Hi Jiri,

    Em Seg, 2007-10-08 *s 13:41 +0100, Jiri Slaby escreveu:
    > cinergyT2, remove bad usage of ERESTARTSYS
    >
    > test of cinergyt2->disconnect_pending doesn't ensure pending signal and so
    > ERESTARTSYS would reach userspace, which is not permitted. Change it to
    > EAGAIN
    >


    checkpatch.pl is complaining about your changeset:

    do not use assignment in if condition
    #82: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:492:
    + if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))

    do not use assignment in if condition
    #86: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:495:
    + if ((err = mutex_lock_interruptible(&cinergyt2->sem)))

    do not use assignment in if condition
    #133: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1036:
    + if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))

    do not use assignment in if condition
    #137: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1039:
    + if ((err = mutex_lock_interruptible(&cinergyt2->sem)))

    Please fix.

    Cheers,
    Mauro

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

    Em Qua, 2007-10-10 *s 00:18 -0400, Michael Krufky escreveu:
    >
    > Is this illegal as per kernel codingstyle?


    Yes, it is. CodingStyle states:

    "Don't put multiple statements on a single line unless you have
    something to hide"
    and
    "Don't put multiple assignments on a single line either. Kernel coding style
    is super simple. Avoid tricky expressions."

    So, Kernel's script/checkpatch.pl is right when complaining about it.

    > I could understand if we may
    > want to avoid this sort of thing for the sake of code readability, but
    > this seems 100% proper to me, especially considering that we're simply
    > trying to catch an error return code.


    > One of the things that I really enjoy about the c programming language
    > is the fact that you can string many operations together into a single
    > statement through the use of logic.


    Yes, this is a great C feature, especially to obfuscate a source code.
    On C, it is possible to write very complex code, with several
    statements, on a single clause, like:

    if((c=(a=x,x-=c,++a)>6?1:-1)>0)goto foo;

    The above code is valid under C, and won't produce a single compiler
    warning. An experienced C programmer will understand the above code,
    while non-experienced ones, even with large experience on other
    programming languages, may take hours to understand.

    A large code, with lots of the above style will be very painful to
    analyze, even for advanced programmers. So, especially on big projects,
    with lots of contributors, this should really be avoided.

    > I hate the thought of a patch being nacked because of the above. :-/
    > If this is indeed kernel codingstyle, then IMHO, we should let it slide
    > for catching error return codes.


    It is just a matter of a simple CodingStyle fix.

    The proper fix is just to replace the offended code by this:

    err=foo();
    if (error)
    goto error;

    --
    Cheers,
    Mauro

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

    On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
    > Em Qua, 2007-10-10 s 00:18 -0400, Michael Krufky escreveu:
    > >
    > > Is this illegal as per kernel codingstyle?

    >
    > Yes, it is. CodingStyle states:



    No.. "Illegal" means prohibited by law. Its merely wrong 8)


    > The proper fix is just to replace the offended code by this:
    >
    > err=foo();
    > if (error)
    > goto error;


    Lots of code uses

    if ((err = foo()) < 0)

    so I would'y worry too much. The split one however clearer and also
    safer.

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS


    Em Qua, 2007-10-10 *s 11:59 -0400, Alan Cox escreveu:
    > On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
    > > Em Qua, 2007-10-10 *s 00:18 -0400, Michael Krufky escreveu:
    > > >
    > > > Is this illegal as per kernel codingstyle?

    > >
    > > Yes, it is. CodingStyle states:

    >
    >
    > No.. "Illegal" means prohibited by law. Its merely wrong 8)
    >


    LOL

    > > The proper fix is just to replace the offended code by this:
    > >
    > > err=foo();
    > > if (error)
    > > goto error;

    >
    > Lots of code uses
    >
    > if ((err = foo()) < 0)
    >
    > so I would'y worry too much. The split one however clearer and also
    > safer.


    Yes, this is not a severe CodingStyle violation. Still, the above code
    is better than the used one.

    Since, on your example, it is clear that the programmer wanted to test
    if the value is less than zero.

    The code:

    if ( (err=foo()) )

    should also indicate an operator mistake of using =, instead of ==.

    Probably, source code analyzers like Coverity will complain about the
    above.

    If not violating CodingStyle, I would rather prefer to code this as:
    if ( !(err=foo() )
    or, even better, using:
    if ( (err=foo()) != 0)

    clearly indicating that it is tested if the value is not zero.

    Even being a quite simple issue, I would prefer if Jiri can fix it.

    --
    Cheers,
    Mauro

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

    Mauro Carvalho Chehab wrote:
    > Em Qua, 2007-10-10 s 11:59 -0400, Alan Cox escreveu:
    >> On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
    >>> Em Qua, 2007-10-10 s 00:18 -0400, Michael Krufky escreveu:
    >>>> Is this illegal as per kernel codingstyle?
    >>> Yes, it is. CodingStyle states:

    >>
    >> No.. "Illegal" means prohibited by law. Its merely wrong 8)
    >>

    >
    > LOL
    >
    >>> The proper fix is just to replace the offended code by this:
    >>>
    >>> err=foo();
    >>> if (error)
    >>> goto error;

    >> Lots of code uses
    >>
    >> if ((err = foo()) < 0)
    >>
    >> so I would'y worry too much. The split one however clearer and also
    >> safer.

    >
    > Yes, this is not a severe CodingStyle violation. Still, the above code
    > is better than the used one.
    >
    > Since, on your example, it is clear that the programmer wanted to test
    > if the value is less than zero.
    >
    > The code:
    >
    > if ( (err=foo()) )
    >
    > should also indicate an operator mistake of using =, instead of ==.
    >
    > Probably, source code analyzers like Coverity will complain about the
    > above.
    >
    > If not violating CodingStyle, I would rather prefer to code this as:
    > if ( !(err=foo() )
    > or, even better, using:
    > if ( (err=foo()) != 0)
    >
    > clearly indicating that it is tested if the value is not zero.
    >
    > Even being a quite simple issue, I would prefer if Jiri can fix it.
    >



    When you have only some few lines of code you can write

    err = foo()
    if (err) {
    do whatever
    }

    doesn't matter ..

    But when you have hell a lot of code, checking error's what you
    mention is insane.

    ie,

    if ((err = foo()) expr ) is better.

    http://lkml.org/lkml/2007/2/4/56

    Manu
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

    Manu Abraham schrieb:
    > Mauro Carvalho Chehab wrote:
    >> Em Qua, 2007-10-10 s 11:59 -0400, Alan Cox escreveu:
    >>> On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
    >>>> Em Qua, 2007-10-10 s 00:18 -0400, Michael Krufky escreveu:
    >>>>> Is this illegal as per kernel codingstyle?
    >>>> Yes, it is. CodingStyle states:
    >>>
    >>> No.. "Illegal" means prohibited by law. Its merely wrong 8)
    >>>

    >> LOL
    >>
    >>>> The proper fix is just to replace the offended code by this:
    >>>>
    >>>> err=foo();
    >>>> if (error)
    >>>> goto error;
    >>> Lots of code uses
    >>>
    >>> if ((err = foo()) < 0)
    >>>
    >>> so I would'y worry too much. The split one however clearer and also
    >>> safer.

    >> Yes, this is not a severe CodingStyle violation. Still, the above code
    >> is better than the used one.
    >>
    >> Since, on your example, it is clear that the programmer wanted to test
    >> if the value is less than zero.
    >>
    >> The code:
    >>
    >> if ( (err=foo()) )
    >>
    >> should also indicate an operator mistake of using =, instead of ==.
    >>
    >> Probably, source code analyzers like Coverity will complain about the
    >> above.
    >>
    >> If not violating CodingStyle, I would rather prefer to code this as:
    >> if ( !(err=foo() )
    >> or, even better, using:
    >> if ( (err=foo()) != 0)
    >>
    >> clearly indicating that it is tested if the value is not zero.
    >>
    >> Even being a quite simple issue, I would prefer if Jiri can fix it.
    >>

    >
    >
    > When you have only some few lines of code you can write
    >
    > err = foo()
    > if (err) {
    > do whatever
    > }
    >
    > doesn't matter ..
    >
    > But when you have hell a lot of code, checking error's what you
    > mention is insane.
    >
    > ie,
    >
    > if ((err = foo()) expr ) is better.
    >
    > http://lkml.org/lkml/2007/2/4/56
    >
    > Manu
    >

    hi manu,

    it's not worth discussing this in a way like
    "i know something from the past and that was a different solution".

    if you look to other parts in that thread like

    http://lkml.org/lkml/2007/2/3/150

    you will see that they came also to a kind of different solution,
    NOT to use the 1-liner for assignment statements.

    it's more like a religious/personal discussion how to
    struct/indent/format code.
    and, to state my position for clear,

    if kernel coding style document isnt updated to allow the constructions
    of code that caused this discussion, we dont have to discuss but follow
    the rules.

    anything else on this topic (coding style and it's sense) is to be
    discussed on kernel ml.

    my 2ct
    marcel



    > _______________________________________________
    > v4l-dvb-maintainer mailing list
    > v4l-dvb-maintainer@linuxtv.org
    > http://www.linuxtv.org/cgi-bin/mailm...dvb-maintainer


    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

    Marcel Siegert wrote:
    > Manu Abraham schrieb:
    >> Mauro Carvalho Chehab wrote:
    >>> Em Qua, 2007-10-10 s 11:59 -0400, Alan Cox escreveu:
    >>>> On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
    >>>>> Em Qua, 2007-10-10 s 00:18 -0400, Michael Krufky escreveu:
    >>>>>> Is this illegal as per kernel codingstyle?
    >>>>> Yes, it is. CodingStyle states:
    >>>>
    >>>> No.. "Illegal" means prohibited by law. Its merely wrong 8)
    >>>>

    >>> LOL
    >>>
    >>>>> The proper fix is just to replace the offended code by this:
    >>>>>
    >>>>> err=foo();
    >>>>> if (error)
    >>>>> goto error;
    >>>> Lots of code uses
    >>>>
    >>>> if ((err = foo()) < 0)
    >>>>
    >>>> so I would'y worry too much. The split one however clearer and also
    >>>> safer.
    >>> Yes, this is not a severe CodingStyle violation. Still, the above code
    >>> is better than the used one.
    >>>
    >>> Since, on your example, it is clear that the programmer wanted to test
    >>> if the value is less than zero.
    >>> The code:
    >>>
    >>> if ( (err=foo()) )
    >>>
    >>> should also indicate an operator mistake of using =, instead of ==.
    >>>
    >>> Probably, source code analyzers like Coverity will complain about the
    >>> above.
    >>>
    >>> If not violating CodingStyle, I would rather prefer to code this as:
    >>> if ( !(err=foo() ) or, even better, using:
    >>> if ( (err=foo()) != 0)
    >>>
    >>> clearly indicating that it is tested if the value is not zero.
    >>>
    >>> Even being a quite simple issue, I would prefer if Jiri can fix it.
    >>>

    >>
    >>
    >> When you have only some few lines of code you can write
    >>
    >> err = foo()
    >> if (err) {
    >> do whatever
    >> }
    >> doesn't matter ..
    >>
    >> But when you have hell a lot of code, checking error's what you
    >> mention is insane.
    >>
    >> ie,
    >>
    >> if ((err = foo()) expr ) is better.
    >>
    >> http://lkml.org/lkml/2007/2/4/56
    >>
    >> Manu
    >>

    > hi manu,
    >
    > it's not worth discussing this in a way like
    > "i know something from the past and that was a different solution".
    >


    didn't mean to look at it that way, because i had addressed my concerns
    at that time as well.

    > if you look to other parts in that thread like
    >
    > http://lkml.org/lkml/2007/2/3/150
    >
    > you will see that they came also to a kind of different solution,
    > NOT to use the 1-liner for assignment statements.
    >
    > it's more like a religious/personal discussion how to
    > struct/indent/format code.
    > and, to state my position for clear,



    It is. Sometimes i find such things in CodingStyle to be too silly.

    >
    > if kernel coding style document isnt updated to allow the constructions
    > of code that caused this discussion, we dont have to discuss but follow
    > the rules.
    >
    > anything else on this topic (coding style and it's sense) is to be
    > discussed on kernel ml.
    >


    Marcel, It is on LKML.

    Manu

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

    Manu Abraham schrieb:
    > Marcel Siegert wrote:
    >> Manu Abraham schrieb:
    >>> Mauro Carvalho Chehab wrote:
    >>>> Em Qua, 2007-10-10 s 11:59 -0400, Alan Cox escreveu:
    >>>>> On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
    >>>>>> Em Qua, 2007-10-10 s 00:18 -0400, Michael Krufky escreveu:
    >>>>>>> Is this illegal as per kernel codingstyle?
    >>>>>> Yes, it is. CodingStyle states:
    >>>>>
    >>>>> No.. "Illegal" means prohibited by law. Its merely wrong 8)
    >>>>>

    >>>> LOL
    >>>>
    >>>>>> The proper fix is just to replace the offended code by this:
    >>>>>>
    >>>>>> err=foo();
    >>>>>> if (error)
    >>>>>> goto error;
    >>>>> Lots of code uses
    >>>>>
    >>>>> if ((err = foo()) < 0)
    >>>>>
    >>>>> so I would'y worry too much. The split one however clearer and also
    >>>>> safer.
    >>>> Yes, this is not a severe CodingStyle violation. Still, the above code
    >>>> is better than the used one.
    >>>>
    >>>> Since, on your example, it is clear that the programmer wanted to test
    >>>> if the value is less than zero.
    >>>> The code:
    >>>>
    >>>> if ( (err=foo()) )
    >>>>
    >>>> should also indicate an operator mistake of using =, instead of ==.
    >>>>
    >>>> Probably, source code analyzers like Coverity will complain about the
    >>>> above.
    >>>>
    >>>> If not violating CodingStyle, I would rather prefer to code this as:
    >>>> if ( !(err=foo() ) or, even better, using:
    >>>> if ( (err=foo()) != 0)
    >>>>
    >>>> clearly indicating that it is tested if the value is not zero.
    >>>>
    >>>> Even being a quite simple issue, I would prefer if Jiri can fix it.
    >>>>
    >>>
    >>> When you have only some few lines of code you can write
    >>>
    >>> err = foo()
    >>> if (err) {
    >>> do whatever
    >>> }
    >>> doesn't matter ..
    >>>
    >>> But when you have hell a lot of code, checking error's what you
    >>> mention is insane.
    >>>
    >>> ie,
    >>>
    >>> if ((err = foo()) expr ) is better.
    >>>
    >>> http://lkml.org/lkml/2007/2/4/56
    >>>
    >>> Manu
    >>>

    >> hi manu,
    >>
    >> it's not worth discussing this in a way like
    >> "i know something from the past and that was a different solution".
    >>

    >
    > didn't mean to look at it that way, because i had addressed my concerns
    > at that time as well.
    >
    >> if you look to other parts in that thread like
    >>
    >> http://lkml.org/lkml/2007/2/3/150
    >>
    >> you will see that they came also to a kind of different solution,
    >> NOT to use the 1-liner for assignment statements.
    >>
    >> it's more like a religious/personal discussion how to
    >> struct/indent/format code.
    >> and, to state my position for clear,

    >
    >
    > It is. Sometimes i find such things in CodingStyle to be too silly.
    >
    >> if kernel coding style document isnt updated to allow the constructions
    >> of code that caused this discussion, we dont have to discuss but follow
    >> the rules.
    >>
    >> anything else on this topic (coding style and it's sense) is to be
    >> discussed on kernel ml.
    >>

    >
    > Marcel, It is on LKML.


    i do know manu, but as far as i can see from my fresh 2.6.23,
    its not solved or changed in vanilla kernel CodingStyle doc.

    so we have to follow actual guidelines _or_ wait until
    CodingStyle is accordingly updated.

    not more, not less.


    regards
    marcel

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

    Mauro Carvalho Chehab wrote:
    > Hi Jiri,
    >
    > Em Seg, 2007-10-08 *s 13:41 +0100, Jiri Slaby escreveu:
    >
    >> cinergyT2, remove bad usage of ERESTARTSYS
    >>
    >> test of cinergyt2->disconnect_pending doesn't ensure pending signal and so
    >> ERESTARTSYS would reach userspace, which is not permitted. Change it to
    >> EAGAIN
    >>
    >>

    >
    > checkpatch.pl is complaining about your changeset:
    >
    > do not use assignment in if condition
    > #82: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:492:
    > + if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
    >
    > do not use assignment in if condition
    > #86: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:495:
    > + if ((err = mutex_lock_interruptible(&cinergyt2->sem)))
    >
    > do not use assignment in if condition
    > #133: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1036:
    > + if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
    >
    > do not use assignment in if condition
    > #137: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1039:
    > + if ((err = mutex_lock_interruptible(&cinergyt2->sem)))


    Is this illegal as per kernel codingstyle? I could understand if we may
    want to avoid this sort of thing for the sake of code readability, but
    this seems 100% proper to me, especially considering that we're simply
    trying to catch an error return code.

    One of the things that I really enjoy about the c programming language
    is the fact that you can string many operations together into a single
    statement through the use of logic. I hate the thought of a patch being
    nacked because of the above. :-/

    If this is indeed kernel codingstyle, then IMHO, we should let it slide
    for catching error return codes.

    Regards,

    Mike Krufky
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

    On Wed, 10 Oct 2007 00:18:28 -0400 Michael Krufky wrote:

    > Mauro Carvalho Chehab wrote:
    > > Hi Jiri,
    > >
    > > Em Seg, 2007-10-08 s 13:41 +0100, Jiri Slaby escreveu:
    > >
    > >> cinergyT2, remove bad usage of ERESTARTSYS
    > >>
    > >> test of cinergyt2->disconnect_pending doesn't ensure pending signal and so
    > >> ERESTARTSYS would reach userspace, which is not permitted. Change it to
    > >> EAGAIN
    > >>
    > >>

    > >
    > > checkpatch.pl is complaining about your changeset:
    > >
    > > do not use assignment in if condition
    > > #82: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:492:
    > > + if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
    > >
    > > do not use assignment in if condition
    > > #86: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:495:
    > > + if ((err = mutex_lock_interruptible(&cinergyt2->sem)))
    > >
    > > do not use assignment in if condition
    > > #133: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1036:
    > > + if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
    > >
    > > do not use assignment in if condition
    > > #137: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1039:
    > > + if ((err = mutex_lock_interruptible(&cinergyt2->sem)))

    >
    > Is this illegal as per kernel codingstyle? I could understand if we may
    > want to avoid this sort of thing for the sake of code readability, but
    > this seems 100% proper to me, especially considering that we're simply
    > trying to catch an error return code.


    Anyway, it's not strictly from CodingStyle. The closest that
    CodingStyle has to say about it IMO is this:

    "Don't put multiple assignments on a single line either. Kernel coding style
    is super simple. Avoid tricky expressions."

    Also, Andrew Morton and Christoph Hellwig push for splitting up
    the if-assignment lines, so it's a trend over the past few
    (probably) years. It's not real new.


    > One of the things that I really enjoy about the c programming language
    > is the fact that you can string many operations together into a single
    > statement through the use of logic. I hate the thought of a patch being
    > nacked because of the above. :-/


    so you like the challenge of reading obfuscated code?

    At any rate, it's rare that a patch is nacked only due to coding style,
    unless it's blatant and occurs many times (like throughout the entire
    source file).


    > If this is indeed kernel codingstyle, then IMHO, we should let it slide
    > for catching error return codes.


    Nope.

    ---
    ~Randy
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread