Usage of 'goto'

[ApplePie420]

Hey, I was just looking around, and found various goto statements in code. I was always told and taught that goto can mess up stuff pretty badly. Just want to ask, is there any reason for doing this:

// ...
    if (OS_VERIFY_SMBIOS3(pMappedAddr))
    {
        status = NV_OK;
        goto done;
    }

    if (OS_VERIFY_SMBIOS(pMappedAddr))
    {
        status = NV_OK;
    }

done:
    os_unmap_kernel_space(pBufAddr, size);
    return status;
// ...

instead of this:

// ...
    if (OS_VERIFY_SMBIOS3(pMappedAddr))
    {
        status = NV_OK;
        s_unmap_kernel_space(pBufAddr, size);
        return status;
    }

    if (OS_VERIFY_SMBIOS(pMappedAddr))
    {
        status = NV_OK;
    }
// ...

Example taken from os.c

I understand that in controlled cases it won't do anything, but still.. also subjectively, it messes up readability of the code.

EDIT: In many cases it enforces "DNRY", and I imagine it would be pain to implement as a function.. but there are some cases where the goto is used only once in the function, or even entire file

[mtijanic]

Thank you for the interest in our codebase.

goto is a tool of the language like any other. Knives are sharper than forks, so you ought to be more careful when using them, but you're gonna have a bad time eliminating all knife use when making food, even if technically doable.

Generally the goto cleanup; idiom is used a lot in C code, as it is the safest way to ensure all cleanup is performed on every function exit path. It's not just a matter of minimizing duplicated lines either. When adding the cleanup calls (s_unmap_kernel_space() in your example) directly before every return, you have two common failure modes:

• You added a new return path, but forgot to perform all cleanup
• You added a new cleanup function, but forgot to update all returns

Our rough guidelines here say to use regular return when validating input parameters before any resource allocations have been performed, and then use goto done; after for all return paths.

There is however a difference in how it is used in nvidia.ko versus what e.g. linux kernel does. Consider this artificial example using kernel style:

void *get_some_resource() {
    A *a;
    B *b;
    C *c;

    a = allocate_a();
    if (!a)
        return ERR_PTR(-ENOMEM);

    b = allocate_b(a);
    if (!b)
        goto err1;

    c = allocate_c(b);
    if (!c)
        goto err2;
    
     return c;
err2:
    free_b(b);
err1:
    free_a(a);
    return ERR_PTR(-ENOMEM);
}

A more idiomatic way to write this in nvidia.ko (other modules have slightly different conventions) would be:

NV_STATUS get_some_resource(void **res)
{
    NV_STATUS status;
    A *a = NULL;
    B *b = NULL;
    C *c = NULL;

    status = allocate_a(&a);
    if (status != NV_OK)
        goto done;

    status = allocate_b(&b, a);
    if (status != NV_OK)
        goto done;

    status = allocate_c(&c, b);
    if (status != NV_OK)
        goto done;

    *res = c;    
done:
    if (status != NV_OK)
    {
        free_b(b); // these are typically NOPs when NULL is passed in
        free_a(a);
    }
    return status;
}

Or with some error-checking macros:

NV_STATUS get_some_resource(void **res)
{
    NV_STATUS status;
    A *a = NULL;
    B *b = NULL;
    C *c = NULL;

    NV_ASSERT_OK_OR_GOTO(status, allocate_a(&a), done);
    NV_ASSERT_OK_OR_GOTO(status, allocate_b(&b, a), done);
    NV_ASSERT_OK_OR_GOTO(status, allocate_c(&c, b), done);

    *res = c;    
done:
    if (status != NV_OK)
    {
        free_b(b);
        free_a(a);
    }
    return status;
}

Notably, we usually only have one label (often called done) and the success path also goes through it. The cleanup functions are all usually in a if (status != NV_OK) {...} block.

There are of course many exceptions to this in the codebase. Guidelines can be ignored whenever there is a strong reason to do so.

[ApplePie420]

Wow, thanks for exhausting response!
So basically if I understood it correctly, it's kinda what callbacks are in JavaScript, but their purpose is just to free up resources.
I still found some examples that does not make much sense to me, why it would be that way (like here for example). It tries and frees stuff, without really returning anything, except at the beginning and the end. And (almost) each time, it frees up different thing with different function..
Looking back at it, maybe I've jumped into kernel modules a bit too quickly :D

[mtijanic]

That one is an excellent example of the first pattern I showed above, with err1 and err2. What's important to know is that goto labels are "fall-through", so in my example above, when you do goto err2; it will run both free_b() and then free_a();

The labels and freeing is in opposite order from allocation. So you just jump to the label that will free all the stuff you already allocated and won't touch the rest.

[ApplePie420]

Ohhh okay, I didn't knew they execute everything under them too. Now it makes a lot more sense. For some reason I kinda assumed it's similar to assembly (since I was taught they are essentially an ASM labels), so that they are "scoped" just to that block (which retrospectively does not make really sense, since C is not indentation sensitive). Thanks a lot for explaining.

[Evernow]

Thank you very much for your response @mtijanic , it's a great response for students who often only get one side of the argument when it comes to goto. Greatly appreciated that you took the time to explain!