Ich habe vor über einem Jahr einen kompletten neuen Treiber als Patch eingesandt. Zu tun hatte ich mit gleich zwei Subsystem-Maintainern, weil der Treiber beides wesentlich berührte.
Aus der einen Ecke kamen wichtige Tipps, wie man das Subsystem richtig benutzt. Umgesetzt, eingebaut. Aus der anderen Ecke nur den Hinweis, dass kein offensichtlicher Fehler zu sehen sei. Also habe ich das eingreicht und geharrt ob der Dinge, die da kommen mögen. Es passierte gar nichts, der Patch lungerte bei einem der Maintainer in der Pipeline, und die Kernelversionen zogen vorbei.
Nach einigen Monaten hab ich dann nachgehakt, und es stellte sich heraus, dass ich mich direkt an die Lkml hätte wenden müssen, weil das mit zwei Maintainern im Kompetenzwirrwarr untergegangen sei. Ich habe mich dnan direkt an Greg Kroah-Hartmann gewandt, der mich etwas harsch anging, weil mein Emailprogramm immerzu die Patches kaputtformatierte. Zum Glück habe ich sein git-send-email gefunden, sonst hätte er mir vermutlich noch den Kopf abgebissen. Dann war er aber sehr freundlich und hat mir geduldig erklärt, dass ich meinen Treiber nochmals an den aktuellen Kernel anpassen müsste. Das habe ich dann gemacht, und jetzt ist er drin.
Mangels Hardware konnten alle drei nichts am lebenden Objekt testen. Das heißt im Klartext, niemand außer mir und den zwei Benutzern, die sich inzwischen direkt bei mir gemeldet haben weiß, ob dieser Treiber funktioniert.
Ein Review fand also überhaupt nicht statt. Ich bin der einzige, der etwas Konkretes weiß.
Dieser Beitrag wurde 1 mal editiert. Zuletzt am 24. Apr 2018 um 14:50.
Kurzform: Du hast Tipps bezüglich Struktur des Subsystems bekommen und es wurde erkannt, das Du noch Anpassungen an den aktuellen Kernel machen musst.
Das klingt für mich sehr wohl nach Review (das ist nicht das Gleiche wie ein Test!!!) - jemand hat mit Verstand Deinen Code angeguckt. Hättest Du erwartet, das die Maintainer jede Änderung intensiv selbst testen? Auf echter Hardware?
Von kamome umidori am Di, 24. April 2018 um 15:26 #
Weißt Du denn, ob zumindest mal jemand über den Code drübergeschaut hat, um wenigstens `if (uid = 0)` und so rauszufangen? Oder hast Du jetzt eine persönliche Backdoor im Kernel?
Ich antworte mir mal selbst, weil hier ja so einige schrieben "Wieso, ein Review fand doch statt."
Bei einem Review erwarte ich, dass ich ein Ergebnis bekomme. Und nicht Hopplahopp hier mal einen Hinweis und da mal einen. Das hat kein System und geht allenfalls als gutgemeinte Ratschläge durch. Review heißt, dass es ein Protokoll gibt, was geprüft wurde und warum.
Dass ein Treiber in irgendeiner Pipeline rumlungert, bis mir der Maintainer als Entwickler auf Nachfrage schließlich mitteilt, er sei ja gar nicht zuständig, zeigt ebenfalls, dass es Lücken im Reviewprozess gibt.
Schließlich habe ich mich direkt an einen der Obergurus gewandt und das einzige Bürokratieproblem war der dumme Mailer. Ich musste gar nichts vorweisen, keinen Leumund, keine Bestätigung irgendeines Reviews. Vielleicht funktioniert das zwischen den Maintainern ja per stiller Post, es ist jedenfalls hochgradig obskur und das Ergebnis nicht nachvollziehbar.
Ob es eine funktionale Prüfung gab weiß auch keiner. Und was keiner weiß, wurde bekanntlich nicht gemacht. Gut, ich habe sie mehrfach gemacht, aber wer weiß das denn?
Wenn es da irgendwo ein System geben sollte, ist es schwer zu entdecken. Mir ist es nicht gelungen. Aber es funktioniert ja irgendwie, also will ich nicht meckern.
Dieser Beitrag wurde 1 mal editiert. Zuletzt am 24. Apr 2018 um 19:55.
Von Fragen über Fragen am Di, 24. April 2018 um 23:09 #
Ein Review ist meiner Ansicht nach eine 2 Ansicht eines anderen Entwicklers. Er kann Dinge erkennen, die du durch Betriebsblindheit übersehen könntest. Aber ein Test ist kein Review und wenn es beim kompilieren keine Probleme macht, scheint es zu funktionieren. Man geht einfach davon aus, das wenn es bei dir läuft, wird es auch bei anderen laufen. Mangels Testgerät wohl die praktikabelste Lösung.
So sehe ich das auch. Ein Review könnte auch bedeuten, den Code ausdrucken, anschauen und als Rückmeldung ein "Ok". Natürlich gibt es bessere Tools als ein papierverschwendes "cat" und eine Rückmeldung darf auch ausführlicher sein.
Meiner Meinung nach muss durch den Code Review sichergestellt werden, dass kein böser Code enthalten ist, dass keine groben Fehler enthalten sind und dass der Code sich an die gemeinsamen Richtlinien hält. Wie es aussieht wurde das in diesem Fall auch gemacht. Aber der ganze Prozess ist nicht gut abgelaufen, nicht gut dokumentiert und bestätigt, dass es wie im Artikel behandelt, in diesem Prozess bei Linux Probleme gibt.
Von Anon Y. Mouse am Mi, 25. April 2018 um 22:25 #
Tja der Punkt ist wie bereits mehrfach angesprochen der unterschied zwischen review und test. Bei einem review wird der code überflogen, mehr oder weniger in die tiefe, bei einem test werden testszenarien etc. Entwickelt und Auf den code/binary angewandt.
Ein review checkt ausschliesslich formales, d.h. du könntest auch einen treiber für nicht existierende hw schreiben, wenn es nichts kaputt macht und ordentlich aussieht wird es durchkommen...
Falls es dich mal interessiert schau dir mal RTCA DO-178B an, danach werden sw für avionik Geräte geschrieben je nach kritikalität von level e (ausfall ist egal) bis level a (ausfall führt zum verlust des Flugzeugs) gibt es da erhebliche Unterschiede. Im bahnbereich gibt es eine EN wo ebenfalls nach ausfall Auswirkungen unterschiedlich geprüft wird.
Ich habe vor über einem Jahr einen kompletten neuen Treiber als Patch eingesandt. Zu tun hatte ich mit gleich zwei Subsystem-Maintainern, weil der Treiber beides wesentlich berührte.
Aus der einen Ecke kamen wichtige Tipps, wie man das Subsystem richtig benutzt. Umgesetzt, eingebaut. Aus der anderen Ecke nur den Hinweis, dass kein offensichtlicher Fehler zu sehen sei. Also habe ich das eingreicht und geharrt ob der Dinge, die da kommen mögen. Es passierte gar nichts, der Patch lungerte bei einem der Maintainer in der Pipeline, und die Kernelversionen zogen vorbei.
Nach einigen Monaten hab ich dann nachgehakt, und es stellte sich heraus, dass ich mich direkt an die Lkml hätte wenden müssen, weil das mit zwei Maintainern im Kompetenzwirrwarr untergegangen sei. Ich habe mich dnan direkt an Greg Kroah-Hartmann gewandt, der mich etwas harsch anging, weil mein Emailprogramm immerzu die Patches kaputtformatierte. Zum Glück habe ich sein git-send-email gefunden, sonst hätte er mir vermutlich noch den Kopf abgebissen. Dann war er aber sehr freundlich und hat mir geduldig erklärt, dass ich meinen Treiber nochmals an den aktuellen Kernel anpassen müsste. Das habe ich dann gemacht, und jetzt ist er drin.
Mangels Hardware konnten alle drei nichts am lebenden Objekt testen. Das heißt im Klartext, niemand außer mir und den zwei Benutzern, die sich inzwischen direkt bei mir gemeldet haben weiß, ob dieser Treiber funktioniert.
Ein Review fand also überhaupt nicht statt. Ich bin der einzige, der etwas Konkretes weiß.
Dieser Beitrag wurde 1 mal editiert. Zuletzt am 24. Apr 2018 um 14:50.da kriegt man es mit der angst zu tun.
Warum schlussfolgerst Du, das kein Review stattgefunden habe?
Das kann ich dem Ganzen nicht entnehmen.
Das die Maintainer nicht jede erdenkliche Hardware herumstehen haben um alles im Realbetrieb zu Testen sollte doch wohl klar sein.
Ein Review Test
Da hat es mir den Beitrag zerhauen.
Kurzform: Du hast Tipps bezüglich Struktur des Subsystems bekommen und es wurde erkannt, das Du noch Anpassungen an den aktuellen Kernel machen musst.
Das klingt für mich sehr wohl nach Review (das ist nicht das Gleiche wie ein Test!!!) - jemand hat mit Verstand Deinen Code angeguckt.
Hättest Du erwartet, das die Maintainer jede Änderung intensiv selbst testen? Auf echter Hardware?
Weißt Du denn, ob zumindest mal jemand über den Code drübergeschaut hat, um wenigstens `if (uid = 0)` und so rauszufangen? Oder hast Du jetzt eine persönliche Backdoor im Kernel?
Ein Rewiew findet doch schon statt wenn es sich einer an/durchsieht was bei dir ja wohl passiert ist.
Ich antworte mir mal selbst, weil hier ja so einige schrieben "Wieso, ein Review fand doch statt."
Bei einem Review erwarte ich, dass ich ein Ergebnis bekomme. Und nicht Hopplahopp hier mal einen Hinweis und da mal einen. Das hat kein System und geht allenfalls als gutgemeinte Ratschläge durch. Review heißt, dass es ein Protokoll gibt, was geprüft wurde und warum.
Dass ein Treiber in irgendeiner Pipeline rumlungert, bis mir der Maintainer als Entwickler auf Nachfrage schließlich mitteilt, er sei ja gar nicht zuständig, zeigt ebenfalls, dass es Lücken im Reviewprozess gibt.
Schließlich habe ich mich direkt an einen der Obergurus gewandt und das einzige Bürokratieproblem war der dumme Mailer. Ich musste gar nichts vorweisen, keinen Leumund, keine Bestätigung irgendeines Reviews. Vielleicht funktioniert das zwischen den Maintainern ja per stiller Post, es ist jedenfalls hochgradig obskur und das Ergebnis nicht nachvollziehbar.
Ob es eine funktionale Prüfung gab weiß auch keiner. Und was keiner weiß, wurde bekanntlich nicht gemacht. Gut, ich habe sie mehrfach gemacht, aber wer weiß das denn?
Wenn es da irgendwo ein System geben sollte, ist es schwer zu entdecken. Mir ist es nicht gelungen. Aber es funktioniert ja irgendwie, also will ich nicht meckern.
Dieser Beitrag wurde 1 mal editiert. Zuletzt am 24. Apr 2018 um 19:55.Ein Review ist meiner Ansicht nach eine 2 Ansicht eines anderen Entwicklers. Er kann Dinge erkennen, die du durch Betriebsblindheit übersehen könntest. Aber ein Test ist kein Review und wenn es beim kompilieren keine Probleme macht, scheint es zu funktionieren. Man geht einfach davon aus, das wenn es bei dir läuft, wird es auch bei anderen laufen. Mangels Testgerät wohl die praktikabelste Lösung.
So sehe ich das auch. Ein Review könnte auch bedeuten, den Code ausdrucken, anschauen und als Rückmeldung ein "Ok". Natürlich gibt es bessere Tools als ein papierverschwendes "cat" und eine Rückmeldung darf auch ausführlicher sein.
Meiner Meinung nach muss durch den Code Review sichergestellt werden, dass kein böser Code enthalten ist, dass keine groben Fehler enthalten sind und dass der Code sich an die gemeinsamen Richtlinien hält. Wie es aussieht wurde das in diesem Fall auch gemacht. Aber der ganze Prozess ist nicht gut abgelaufen, nicht gut dokumentiert und bestätigt, dass es wie im Artikel behandelt, in diesem Prozess bei Linux Probleme gibt.
@Janka: Danke für deinen Einblick in den Ablauf.
Ich habe mal an Stadtbahnstellwerken mitentwickelt, vermutlich bin ich davon einfach noch systematischeres Vorgehen und härtere Prüfungen gewöhnt.
Tja der Punkt ist wie bereits mehrfach angesprochen der unterschied zwischen review und test. Bei einem review wird der code überflogen, mehr oder weniger in die tiefe, bei einem test werden testszenarien etc. Entwickelt und Auf den code/binary angewandt.
Ein review checkt ausschliesslich formales, d.h. du könntest auch einen treiber für nicht existierende hw schreiben, wenn es nichts kaputt macht und ordentlich aussieht wird es durchkommen...
Falls es dich mal interessiert schau dir mal RTCA DO-178B an, danach werden sw für avionik Geräte geschrieben je nach kritikalität von level e (ausfall ist egal) bis level a (ausfall führt zum verlust des Flugzeugs) gibt es da erhebliche Unterschiede. Im bahnbereich gibt es eine EN wo ebenfalls nach ausfall Auswirkungen unterschiedlich geprüft wird.
Auch bei einem Review muss man einem Prozess folgen. Sonst ist das gar nix.
Denn man braucht nicht die Hardware um einen Code Audit durchzuführen.
Wenn du jetzt noch den Treiber und die betroffene Hardware nennst, dann sind es vielleicht mehr Leute, die dazu noch etwas sagen können.