2
\$\begingroup\$

As many of you know goto is usually signs of code smell. However I thought this could be an appropriate case, and would like confirmation or criticism.

Unnecessary section such as the called functions were removed. Every non-standard function returns an int as status, 0 is "OK"; except linkedlist_open() which returns a pointer which could be NULL if the system runs out of memory.

packagetarget_close() does implement the necessary NULL checks.

packagetarget *packagetarget_open()
{
    packagetarget *target = (packagetarget*) malloc(sizeof(packagetarget));

    if (!target) return NULL;

    if (packagetarget_setname(target, ""))
        goto disposer;
    if (packagetarget_setsys(target, PACKAGETARGET_SYS))
        goto disposer;
    if (packagetarget_setarch(target, PACKAGETARGET_ARCH))
        goto disposer;
    if (packagetarget_setmin(target, PACKAGETARGET_MIN))
        goto disposer;
    if (packagetarget_setver(target, PACKAGETARGET_VER))
        goto disposer;
    if (packagetarget_setmax(target, PACKAGETARGET_MAX))
        goto disposer;

    linkedlist *comp = linkedlist_open();

    if (!comp) goto disposer;

    target->comp = comp;

    return target;   

    disposer:
        packagetarget_close(target);
        return NULL;
}

Each setter function follows a simple pattern. Since each setter is identical, I will only put the code for packagetarget_setname.

int packagetarget_setname(packagetarget *target, char *name)
{
    if (!target) return 1;

    if (!name) return 2;

    target->name = realloc(target->name, strlen(name) * sizeof(char));

    if (!target->name) return 3;

    strcpy(target->name, name);

    return 0;
}

Here is the packagetarget_close function:

void packagetarget_close(packagetarget *target)
{
    if (!target) return;

    if (target->name) free(target->name);
    if (target->sys) free(target->sys);
    if (target->arch) free(target->arch);
    if (target->min) free(target->min);
    if (target->ver) free(target->ver);
    if (target->min) free(target->min);
    if (target->comp) linkedlist_close(target->comp);

    free(target);

    return;
}

linkedlist_open is a similar function to the packagetarget_open.

linkedlist *linkedlist_open()
{
    linkedlist *list = (linkedlist*) malloc(sizeof(linkedlist));

    if (!list) return NULL;

    list->head = NULL;
    list->length = 0;
    list->remhook = NULL;

    return list;
}

The one thing most of these functions is they may fail when the system runs out of memory. So I have implemented checks for each step.

\$\endgroup\$
  • \$\begingroup\$ "However I thought this could be an appropriate case, and would like confirmation or criticism." Yet you've stripped the code out of all it's context so we can't determine whether you're right or not. The code you've provided looks like it should've been used in a wrapper, not with goto constructs. Please provide more code and an explanation of what it's supposed to do so we can see how this snippet is being used. Stack Overflow likes minimal examples, Code Review does absolutely not. Please take a look at our FAQ on asking questions. \$\endgroup\$ – Mast Mar 30 at 21:09
  • \$\begingroup\$ Not all of the functions are written, yet. This function is a constructor for a struct named packagetarget. The idea is to allocate the struct in memory, and then call the setter methods for each field, which may fail. EDIT: If any step fails, the destructor function is called, once; and the function returns a null pointer. \$\endgroup\$ – utkumaden Mar 30 at 21:11
  • \$\begingroup\$ If not all the functions are written, are you sure it works the way it should? And how can you be sure this is the right way to do it if you haven't completed the rest? It sounds like you're simply too early in the process got get this meaningfully reviewed and your question answered. \$\endgroup\$ – Mast Mar 30 at 21:15
  • \$\begingroup\$ I have updated the question to include more code. \$\endgroup\$ – utkumaden Mar 30 at 21:27
  • 1
    \$\begingroup\$ What is a constructor in C? I know what a constructor is in C++. Did you mean C++? \$\endgroup\$ – pacmaninbw Mar 30 at 22:12
3
\$\begingroup\$

packagetarget_close()

First of all, you have a copy-and-paste error in packagetarget_close(), where you attempt to free target->min twice, but not target->max.

Next, note that most of the if statements are superfluous. As per the standard behaviour for free(),

If ptr is a null pointer, the function does nothing.

packagetarget_open()

You have undefined behaviour due to the way your error-handling works. The chunk of memory returned by malloc() contains arbitrary junk. If packagetarget *target = (packagetarget*) malloc(sizeof(packagetarget)) succeeds, but one of the setters fails, then you would call packagetarget_close(), which would then interpret that arbitrary junk as pointers to memory to be freed. A good way to fix that is to zero the memory before calling any of the setters. You can either use calloc() instead of malloc(), or memset(), or a struct initializer.

Avoiding malloc()

In C, I prefer the init-cleanup idiom over new-destroy (which you call open-close). In the init-cleanup idiom, the caller is responsible for providing the chunk of memory to be initialized, which gives the caller the option of providing either stack-based or heap-based memory.

Avoiding goto

While indiscriminate use of goto leads to spaghetti code, there are some circumstances where goto is justifiable, if used in a readily recognizable pattern.

I think that your use of goto isn't horrible, but personally I would prefer to write the if statements as a chain of && expressions.

Suggested solution

#include <stdlib.h>
#include <string.h>

typedef struct packagetarget {
    char *name;
    char *sys;
    char *arch;
    char *min;
    char *ver;
    char *max;
    linkedlist comp;
} packagetarget;

int packagetarget_setname(packagetarget *target, const char *name) {
    if (!target) return 1;
    if (!name) return 2;
    target->name = realloc(target->name, strlen(name) * sizeof(char));
    if (!target->name) return 3;
    strcpy(target->name, name);
    return 0;
}

int packagetarget_setsys(packagetarget *target, const char *sys) {
    …
}

int packagetarget_setarch(packagetarget *target, const char *arch) {
    …
}

int packagetarget_setmin(packagetarget *target, const char *min) {
    …
}

int packagetarget_setver(packagetarget *target, const char *ver) {
    …
}

int packagetarget_setmax(packagetarget *target, const char *max) {
    …
}

packagetarget *packagetarget_cleanup(packagetarget *target) {
    if (target) {
        free(target->name);
        free(target->sys);
        free(target->arch);
        free(target->min);
        free(target->ver);
        free(target->max);
        linkedlist_cleanup(&target->comp);
    }
    return target;
}

packagetarget *packagetarget_init(packagetarget *target) {
    static const packagetarget empty = {};
    if (target) {
        *target = empty;
        if (!(0 == packagetarget_setname(target, "") &&
              0 == packagetarget_setsys(target, PACKAGETARGET_SYS) &&
              0 == packagetarget_setarch(target, PACKAGETARGET_ARCH) &&
              0 == packagetarget_setmin(target, PACKAGETARGET_MIN) &&
              0 == packagetarget_setver(target, PACKAGETARGET_VER) &&
              0 == packagetarget_setmax(target, PACKAGETARGET_MAX) &&
              linkedlist_init(&target->comp))) {
            packagetarget_cleanup(target);
            return NULL;
        }
    }
    return target;
}

int main() {
    packagetarget t;
    if (packagetarget_init(&t)) {
        …
    }
    packagetarget_cleanup(&t);
}
\$\endgroup\$
  • \$\begingroup\$ I did find the undefined behavior in packagetarget_close() myself too, but it was very late so I couldn't edit the question to fix it. Since I am very early in the project I may switch over idioms like you suggested because it made more sense. I used the open/close idioms because the only c api I am familiar with was Lua, which has the same structure. Also, marked as answer. \$\endgroup\$ – utkumaden Mar 31 at 4:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.