Semaphores + error checking == uh oh.



  • static ssize_t vloopback_write(struct file f, const char buf, size_t count, loff_t offset)
    {
    struct video_device loopdev=video_devdata(f);

    /
    snip a bunch of initialization code /

    if (!waitqueue_active(&loops[nr]->wait)) {
    loops[nr]->framesdumped++;
    return realcount;
    }

    down(&loops[nr]->lock); /
    Okay, so let's use a mutex ...
    /
    if (!loops[nr]->buffer) {
    up(&loops[nr]->lock); /
    * Release it to report the error condition /
    return -EINVAL;
    }
    if (realcount > loops[nr]->buflength) {
    realcount = loops[nr]->buflength;
    }
    if (copy_from_user(
    loops[nr]->buffer+loops[nr]->frame
    loops[nr]->buflength,
    buf, realcount
    )) return -EFAULT; /
    * Oops, couldn't accept user data, return
    error condition, of course, WITH MUTEX HELD /
    loops[nr]->frame=0;
    up(&loops[nr]->lock); /
    Now release the mutex :( /

    wake_up(&loops[nr]->wait);

    /
    snip rest of function ***/

    This module was filled with many similar situations... half of the parameter checks would fail with a mutex held that would lock the pipe on next access. I guess they took "goto considered harmful" to heart. And this is one of many reasons why this otherwise kernel module hasn't been submitted for inclusion...



  • [quote user="hk0"]

    I guess they took "goto considered harmful" to heart.

    [/quote]

    No, they're just inept. The correct way to write sane, goto-free code with locks and without exceptions is to use a wrapper function (just vary the types as necessary):

    void* func(void* args) {

      down(lock);
      void* result = real_func(args);
      up(lock);

      return result;

    Vastly easier to understand what this does, and you can see right away that the locking behaviour of this function is correct. Any piece of code that uses locks can be rearranged in this manner (by breaking it up into many functions), and it's almost always an improvement over burying the locks in a string of ifs or gotos. If you make real_func a static inline function then the compiler will generate the same code as if they were all in one function, but you can be sure that it's correct. There's a handful of cases (involving complex loops and non-symmetric unlocking sequences) where this method is too ugly to be used, but it's very rare in practice.

    If you do it with gotos, you have to carefully check that each goto is jumping out to the right exit point - people get that wrong all the time.

    (If your language has compiler-managed exceptions with correct unwinding, you can use those to generate the same effect, that's fine too. If your language is like C and only has manual exceptions, stick with inline functions.) 



  • Another solution is:

     

    void *func(void args) {

      down(lock);

      do {

        / Do stuff here /

        if(error_condition) break;

        / Do more stuff here */

      } while(0);

      up(lock);

      return result;

    }

     

    But sometimes you have to get constructive with breaking out of internal loops etc. 

     



  • [quote user="GettinSadda"]

    Another solution is:

     

    void *func(void args) {

      down(lock);

      do {

        / Do stuff here /

        if(error_condition) break;

        / Do more stuff here */

      } while(0);

      up(lock);

      return result;

    }

     

    But sometimes you have to get constructive with breaking out of internal loops etc. 

     

    [/quote]

    No! Bad Programmer! Didn't you see my post on this a few weeks ago?

    The do/break/while(0) construct is much worse than a goto, since it's hard to figure out where you go!

    Besides, with C++ exceptions, you can't be sure that you'll ever call "up(lock)"

     



  • The best solution is often Resource Acquisition Is Initialization. Or usually just RAII for short. In languages that do not really support known object time destruction it can be simulated e.g. in Java

     
    class LockRegion

    {

        public LockRegion() {

            lock_.obtain();

        }

        public void release() {

            lock_.release();

        }

        private Mutex lock_;

        };

     
    int foo() {

        LockRegion lock = new LockRegion();

        // nothing between here and the start of the try block

        try {

            // operations with as many throws or returns as you like

        }

        finally {

            lock.release()

        }

    }

     
    Alternatively you can assign null to lock and then obtain it as the first operation in the try block, but the above is slightly simpler.



  • I don't do C++ because it all looks like a WTF to me.



  • [quote user="djork"]I don't do C++ because it all looks like a WTF to me.[/quote]

     Awww... is somebody confused by aww the widdle braces??

    What you need is some 

    #define begin { 

    #define end }

     

    :)



  • @zip said:

    [quote user="djork"]I don't do C++ because it all looks like a WTF to me.

     Awww... is somebody confused by aww the widdle braces??

    What you need is some 

    #define begin { 

    #define end }

     

    :)

    [/quote]

    I'm fine with curly braces. In fact, I love plently of languages with braces. I just think C++ adds such an enormous number of language features/syntax to C that you'll never fit it in your head.

    const type * const var = address ???



  • The first odd thing is that the code I wrote was Java, not C++.  Although I am much more comfortable in C++ so I may have got the syntax wrong somewhere.  C++ does not have the finally keyword.

    The second is that the following code is using the const features added to ANSI C, the pre C99 version around since at least the early nineties.  Are you really still using K+R style? WTF :)

    const type * const var = address ???

    [quote user="djork"]

    I just think C++ adds such an enormous number of language features/syntax to C that you'll never fit it in your head.

    [/quote]

    The language is more complex, but interestingly this allows you to write much simpler code.  Although of course you can still write unreadable rubbish or FORTRAN in any language.



  • [quote user="djork"]

    I'm fine with curly braces. In fact, I love plently of languages with braces. I just think C++ adds such an enormous number of language features/syntax to C that you'll never fit it in your head.

    const type * const var = address ???

    [/quote]

    When I first learned c++, that was my reaction, too: these people are thinking up crazy shit no one would ever need, just for the sake of making my life complicated.

    As I began to use the language, however, I discovered that every feature exists to solve a real-world problem, and just about every one has been exactly what I needed at some point. Now I just wish they had gone further in a few areas...



  • It comes down to personal preference. I prefer learning my languages to be like learning Go, others prefer them to be like learning Warhammer 40,000. :)



  • I should mention (if it isn't obvious) that this is C, running as a driver in the OS. No try/catch/finally. :-( You can setjmp and longjmp if you want, but eh...
    (Incidentally this is a part of a video capture "tee" driver I helped clean up).
     Most other code written in standard kernel style goes like this:

    acquire_mutex(&some_datastructure);

    if(!do_sanity_check_1(some_datastructure)) {
      set return_code; goto error_out;
    }

    if(!do_sanity_check2(some_datastructure)) {
        set return_code; goto error_out;
    }

    modify(some_datastructure)
    return_code = success

    error_out:
    release_mutex(&some_datastructure)

    return return_code;
     

    The mutex is held across "sanity checks" before the main modification because later modifications require the conditions of the initial checks to hold true. 



  • The real WTF is people replying suggesting C++ solutions, when this is pretty obviously C code. I'd guess something to do with Video4Linux.



  • @bugmenot said:

    The real WTF is people replying suggesting C++ solutions, when this is pretty obviously C code. I'd guess something to do with Video4Linux.

    My bad :)



  • [quote user="asuffield"]void* func(void* args) {

      down(lock);
      void* result = real_func(args);
      up(lock);

      return result;

    }[/quote]

    Quoted for truth!

    And The Real WTF (TM) is surely that 'down' increments the lock count, and 'up' decrements it ...



  • [quote user="djork"]

    It comes down to personal preference. I prefer learning my languages to be like learning Go,

    [/quote]

     

    You think it's going to be easy, then find it will take several lifetimes. 



  • Down vs. Up

    Semaphores can be used to make sure no more than "N" entities can hold an object. You initialize the semaphore to the "capacity", and down operations decrement this counter. When you hit zero, you block.

    Typically a semaphore is initialized to one since most people use it like a mutex. 



  • [quote user="bugmenot"]
    The real WTF is people replying suggesting C++ solutions, when this is pretty obviously C code.[/quote]

    Well, that didn't surprise me. You learn proper syncronization and just have to share it with the whole world (cause there are many people who get that very wrong) -- so what if they didn't recognize the target environment. It's a good conversation to have for the sake of having it.

    [quote user="bugmenot"]I'd guess something to do with Video4Linux.[/quote]

    Yeah, I didn't want an initial reaction of "lol lunix" that you sometimes see here... I tried to keep it looking ambigouous. But it underscores the importance of doing it right, because the way it was before, you'd get processes you couldn't get rid of (req. reboot) and fun stuff like that. It gets even hairier because it's a producer-consumer type driver, and it's hard to do that correctly when you are trying to minimize copies from source to sinks and time spent sleeping on frame availability.



  • [quote user="hk0"]

    Semaphores can be used to make sure no more than "N" entities can hold an object. You initialize the semaphore to the "capacity", and down operations decrement this counter. When you hit zero, you block.

    Typically a semaphore is initialized to one since most people use it like a mutex. 

    [/quote]

    The best mutex is obviously a file opened for exclusive writing. If it's good enough for a web application it's good enough for the kernel!



  • [quote user="foxyshadis"][quote user="hk0"]

    Semaphores can be used to make sure no more than "N" entities can hold an object. You initialize the semaphore to the "capacity", and down operations decrement this counter. When you hit zero, you block.

    Typically a semaphore is initialized to one since most people use it like a mutex. 

    [/quote]

    The best mutex is obviously a file opened for exclusive writing. If it's good enough for a web application it's good enough for the kernel!

    [/quote]


    HANDS OFF THE KERNEL, RIGHT NOW!

    PUT THEM UP IN THE AIR WHERE I CAN JOLLY WELL SEE THEM!!

    STEP BACK AND AWAIT SUMMARY EXECUTION!!!

    So how were you going to get exclusive access to a file on a multi process system? You need to get a lock, so we open up a file exclusively. But wait we need a lock to open that file up exclusively so we need to ...

    It might work for a web server, because it can indirectly get the kernel to do what it really wanted to do in the first place. It doesn't work in many parts of the kernel. Sometimes because there are parts of the kernel which don't understand the concept of a file. Sometmes because they would create a cycle like i've illustrated just before, but mostly because it's just too damn expensive. Especially when there are dozens of nicer aleternatives.



  • I think it's pretty plain that  foxyshadis was joking. But yes, should someone actually believe that your post would be in full effect, all up in the house.

     


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.