Windows System Software -- Consulting, Training, Development -- Unique Expertise, Guaranteed Results

Before Posting...
Please check out the Community Guidelines in the Announcements and Administration Category.

The new wil library for C++ code in drivers

2»

Comments

  • Peter_Viscarola_(OSR)Peter_Viscarola_(OSR) Administrator Posts: 7,251

    blocks of error handling that bloat short methods into monsters

    Hmmmm... but I would argue one wants to see all that code. It's there, you're responsible for making it work... might as well have it nice and clear and "in front of your face", no??

    Peter

    Peter Viscarola
    OSR
    @OSRDrivers

  • Vadim_SirotnikovVadim_Sirotnikov Member Posts: 21
    edited May 29
    When it has meaningful logic, sure.
    But plenty of times it's a routine of log, set error and jump to cleanup, but it adds up.

    I guess maybe the golden path would be
    ~~~
    status = Func(...)
    if(status)
    LOG_AND_RETURN(...)
    ~~~
  • Peter_Viscarola_(OSR)Peter_Viscarola_(OSR) Administrator Posts: 7,251

    I see your point, I really do. And that's a LOT better, in my view, than burying the call AND the error handling behind one big macro. But, I've gotta tell you, even this suggestion doesn't make me happy. If I inherit this code from you, it leaves me with questions:

    a) What status actually gets returned (we can assume it's "status" but... do we know that for sure?

    b) What flag and level is associated with this particular log output?

    I am, perhaps, irretrievably old-school when it comes to such things.

    If your point of view is that you want to minimize the intrusiveness of a lot of the boiler-plate error handling, I can see that. But I'd maintain that this is code, while not certainly particularly interesting code, is still code that we own, and that we have to make work correctly, and some port bastard in the future has to maintain. I'm in favor of everything being nice, clear, and explicit. Heck, I'm the guy who puts parens in operations where the precedence order is "obvious" -- I most often favor separating the declaration of a local variable from its initialization "for the sake of clarity." Even the code that Mr. Tippet previously showed:

        if (STATUS_SUCCESS != (Status = WdfObjectCreate(. . ., &Object)))
            goto Cleanup;
    

    makes me absolutely nuts. I have to think about what this code says. Why not just be explicit and clear:

        Status = WdfObjectCreate(. . ., &Object);
    
        if (Status != STATUS_SUCCESS) {
    
            goto Cleanup;
    
        }
    

    Doesn't that scan faster and take less brain-power to decipher?

    (I'm ignoring the issue of whether we should be checking for STATUS_SUCCESS specifically here, I'm not trying to pick on the example)

    I recently worked on the port of a Linux driver project, where everything in the original Linux code was a freakin' macro... and macros within macros. It sure made the code attractive to look at, but it was a complete nightmare to understand what it was actually doing at a detail level.

    Again, let me be quick to acknowledge that there are good engineers who think differently on this topic.

    Peter

    Peter Viscarola
    OSR
    @OSRDrivers

  • Jeffrey_Tippet_[MSFT]Jeffrey_Tippet_[MSFT] Member - All Emails Posts: 520

    Even the code that Mr. Tippet previously showed [...] makes me absolutely nuts.

    I hate that particular style too. I had pulled some filthy tricks to squeeze the code into less vertical space, as I'm aware that my posts here already consume a lot of space. And as you know, newspapers charge by the column-centimeter or column-inch.

    I'm ignoring the issue of whether we should be checking for STATUS_SUCCESS specifically here

    I've gotten in that habit, actually. I've gradually come around to the conclusion that all the other "success" codes are weird special cases that should be immediately handled and not bubbled up through general code. E.g., what would it mean for a generic middleware routine to return STATUS_ALERTED or STATUS_TIMEOUT? In some of my code, I actually have an error-checking macro that expands out to an assertion that the status is either exactly STATUS_SUCCESS or NT_ERROR(x) != FALSE. I'd hate for a STATUS_PENDING to come flying out of some IRP handler and bubble up through layers that weren't expecting to see it.

  • Vadim_SirotnikovVadim_Sirotnikov Member Posts: 21
    edited May 29
    >
    > @"Peter_Viscarola_(OSR)" said:
    > If I inherit this code from you, it leaves me with questions:
    >
    > a) What status actually gets returned (we can assume it's "status" but... do we know that for sure?
    >
    > b) What flag and level is associated with this particular log output?
    >

    You're right off course. It should be clear from name and parameters what is logged where, and what the return value is. I composed that example in haste just to make the point, of how a more concise, declarative style could look like.

    On a side note, I really appreciate the the resulting discussion.
  • Tim_RobertsTim_Roberts Member - All Emails Posts: 13,002
    via Email
    Peter_Viscarola_(OSR) wrote:
    > If I inherit this code from you, it leaves me with questions:

    To be honest, I'm guessing that if almost ANY of us got hold of a bunch
    of code from almost any of the others, we'd gasp in horror. And yet,
    somehow, works gets done...

    Tim Roberts, [email protected]
    Providenza & Boekelheide, Inc.

  • Michal_VodickaMichal_Vodicka Member - All Emails Posts: 51
    edited May 30

    I have to think about what this code says. Why not just be explicit and clear

    What about a compromise?

    if ((Status = WdfObjectCreate(. . ., &Object)) != STATUS_SUCCESS) {
        goto Cleanup;
    }
    

    This is my way. I see the advantage of switching left and right sides of the condition but for me is negligible and the advantage of seeing real action at first prevails.

    To be honest, I'm guessing that if almost ANY of us got hold of a bunch
    of code from almost any of the others, we'd gasp in horror.

    My experience is different. I have to work with a lot of others' code and yet usually don't gaps in horror. Sometimes I do but it is because the code is miserable. For example I don't like standard MS WDK style but still can easily read it. Currently I'm working with RTOS sources which use own rather unusual style and still clean and readable. All what matters is consistency and if the developer was lazy or not. Using "clever" macros to make code shorter is a sign of laziness for me. TANSTAAFL, the price is code readability and it is the bad price. The code should be readable as a book, descriptive enough to read it without skipping back and forth trying to decode what macros really do. Such a code is longer and takes more time to write it but it pays off later many times.

Sign In or Register to comment.

Howdy, Stranger!

It looks like you're new here. If you want to get involved, click one of these buttons!

Upcoming OSR Seminars
Developing Minifilters 29 July 2019 OSR Seminar Space
Writing WDF Drivers 23 Sept 2019 OSR Seminar Space
Kernel Debugging 21 Oct 2019 OSR Seminar Space
Internals & Software Drivers 18 Nov 2019 Dulles, VA