[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Lost] [Patch] Service-Manager



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?