Re: [Lost] [Patch] Service-Manager

[Kevin Wolf <mail@xxxxxxxxxxxxx>]

Antoine Kaufmann schrieb:
> +
> +pid_t service_pid(const char* name)
> +
> +{

Überflüssige Leerzeile. Und dann hätte ich noch was erwartet, was mit 
/** anfängt und mit */ aufhört...

> +static void rpc_needserv(pid_t pid, dword cid, size_t data_size, void* data)
> +{
> +    char name[data_size + 1];
> +    bool result;
> +    memcpy(name, data, data_size);
> +    name[data_size] = 0;

Ist dieses memcpy nicht überflüssig? Ich mein, man könnte ja einfach 
prüfen, ob data auch brav nullterminiert ist.

> +    if (config_done) {
> +        result = service_start(name);
> +    } else {
> +        // Noch nicht konfiguriert. Jetzt bleibt nur hoffen, dass der Dienst
> +        // schon geladen wurde, sich aber noch nicht registriert hat.
> +        result = service_wait(name, 10000);

Wenn die Funktionalität in init wäre, müßte man nicht hoffen, sondern 
könnte wissen.

> +// Liste mit den Services die gestartet werden muessen, bevor die Konfiguration
> +// ausgelesen werden kann.
> +list_t* statup_services;

Da felt ein Buchstabe im Namen. ;-)

> +static void parse_params(int argc, char* argv[])
> +{
> +    int i;
> +
> +    if (argc > 1) {
> +        // Der erste Parameter ist wie unten beschreiben das Root-Verzeichnis
> +        root_dir = argv[1];
> +        chdir(root_dir);
> +
> +        // Die weiteren sind Services auf die gewartet werden muss.
> +        statup_services = list_create();
> +        for (i = 2; i < argc; i++) {
> +            list_push(statup_services, argv[i]);
> +        }
> +    }
> +}

Keine else? Das Ding kann man auch ohne Parameter aufrufen?

> +/**
> + * Auf Statup-Services warten
> + *
> + * @return TRUE wenn alle gestartet werden konnten, FALSE sonst.
> + */
> +static bool wait_startup_services()
> +{
> +    const char* service;
> +    int i;
> +
> +    for (i = 0; (service = list_get_element_at(statup_services, i)); i++) {
> +        if (!service_wait(service, 5000)) {
> +            return FALSE;
> +        }
> +    }

Dieser Algorithmus ist 0xd00f. Warte lieber 5*list_size Sekunden für 
alle Services zusammen anstatt hintereinander jeweils 5 Sekunden. Sonst 
kommt es auf die Reihenfolge an, wer wieviel Zeit hat.

> +/**
> + * Hauptfunktion
> + *
> + * Vorgesehene Parameter fuer den Programmaufruf:
> + *  - Root-Verzeichnis (Konfiguration in $ROOT/config/servmgr/)
> + *  - Services auf die gewartet werden muss 
> + */
> +int main(int argc, char* argv[])
> +{
> +    // Parameter verarbeiten
> +    parse_params(argc, argv);
> +    
> +    init_service_register("servmgr");
> +
> +    // RPC-Interface vorbereiten
> +    rpcif_init();

Das init_service_register() gehört unter die Registrierung der 
RPC-Funktionen. Wahrscheinlich sogar ganz runter direkt vor die 
Endloschleife.

> +static void config_read_file(const char* path, char** ptr)
> +{
> +    FILE* f = fopen(path, "r");
> +    if (!f) {
> +        *ptr = NULL;
> +    } else {
> +        // FIXME: Das koennte man noch etwas eleganter loesen ;-)
> +        char buffer[513];
> +        size_t size;
> +        
> +        size = fread(buffer, 1, sizeof(buffer) - 1, f);
> +        buffer[size] = 0;
> +        
> +        // Einen eventuellen Zeilenumbruch entfernen
> +        if (buffer[size - 1] == '\n') {
> +            buffer[size - 1] = 0;
> +        }
> +
> +        assert(asprintf(ptr, "%s", buffer) != -1);

Das tut so nicht, Stichwort NDEBUG.

Machst du noch einen zweiten Patch, der den Standard-0.2.0-Bootprozeß 
auf servmgr umstellt?