Neue C++ ORM Bibliothek "oos" v0.2.1
-
Hallo Mechanics,
vielen dank für Deine Antwort. Mal sehen, ob ich Deine Bedenken zerstreuen und ein paar Dinge aufklären kann.
Mechanics schrieb:
Hmm, irgendwie hast du mich noch nicht wirklich überzeugt. Ich hab mir auch die Online Doku angeschaut, da steht aber nicht viel mehr als in deinem Beispiel hier.
Das Beispiel ist so ziemlich das einfachste Beispiel was man mit der Bibliothek realisieren kann.
Sie mal in der Doku, da stehen noch viele andere Dinge: http://zussel.github.io/oos/api/index.htmlMechanics schrieb:
Was mir schon mal nicht wirklich gefällt ist, dass man von einer Basisklasse ableiten muss. Man kann natürlich drüber streiten und es mag Vor- und Nachteile haben, aber das ist schon mal recht unschön.
Das Ableiten von einer Basisklasse ist wohl oder übel notwendig, damit das Objekt auch korrekt serialisiert werden kann. C++ bietet keine Reflections wie in Java, deshalb finde ich diese Variante gar nicht so schlimm.
Wie würdest Du denn das generische Serialisieren ohne Ableiten realisieren?
Mechanics schrieb:
Was ist mit komplex verschachtelten Klassen? Du hast nichts darüber geschrieben (nicht mal unter Future Plans?), dafür steht unter limitations, dass date und time Klasse nicht unterstützt werden. Verschachtelte Klassen sind aber eigentlich ein KO Kriterium, so einfache Klassen wie in deinem Beispiel sind nicht wirklich praxisnah. Was ist mit Beziehungen zwischen Objekten? Das wäre natürlich auch sehr wichtig. Ich würd gern zumindest sehen, dass dir die Probleme/Anforderungen klar sind und du dir Gedanken dazu gemacht hast.
Ok, dass kam bei dem einfachen Bespiel nicht rüber. Um Dich zu beruhigen: Jepp, geht alles, von verschachteteln Klassen über Listen und Vektoren. Schau mal hier: http://zussel.github.io/oos/api/relations.html
Um das Beispiel zu erweitern, spendieren wir unserer Person Klasse noch einen Kumpel (was auch eine Person ist):
class person : public oos::object { // ... bisheriger kram // ... Kumpel typedef oos::object_ref<person> person_ref; person_ref mate_; };
Wie Du siehst geht es recht einfach. Generell arbeitet man nicht auf dem rohen Objekt sondern mit einem
object_ptr
oder einem
object_ref
.
Unterschied ist hier, das ersterer beim löschen mitgelöscht wird und letzterer nicht. Damit kann man kaskadiertes löschen realisieren.
Mechanics schrieb:
Die API vom "ObjectStore" find ich jetzt aber tatsächlich nicht schlecht, vielleicht kann man das ganze noch ausbauen.
Freut mich, dass Dir die API gefällt, vielleicht magst Du sie ja auch mal ausprobieren.
Gruß
Sascha
-
Hallo cooky451,
waoh, dass ist ja mal direkt ein konkreter Verbesserungsvorschlag! Vielen Dank dafür. Mit dem Handling von Transaktionen bin ich in der Tat noch nicht ganz zufrieden (ist halt Version 0.1.0)
Mein nächster Ansatz würde in die Richtung RAII gehen. Dann würde das ungefähr so aussehen:
{ oos::transaction tr(db); // beim anlegen der Transaktion wird automatisch ein begin() aufgerfufen ostore.insert(new person("George")); ostore.insert(new person("Jane")); ostore.insert(new person("Tim")); ostore.insert(new person("Walter")); // wird der Scope verlassen wird beim zerstören der Transaktion // automatisch commit() und je nachdem ein rollback() aufgerufen. }
Aber Deine Variante hat in der Tat auch seine Vorzüge!
Was Multithreading angeht, so habe ich darüber nur am Rande nachgedacht. Als nächstes möchte ein weiteres Datenbank Backend zur Verfügung stellen, wahrscheinlich MSSQL und kleinere Dinge anpassen (so wie das Transaktion handling).
Gruß
Saschacooky451 schrieb:
Was passiert wenn eine transaction out-of-scope geht und weder commit noch rollback aufgerufen wurden? Als einfachen Fix könnte man sagen dass der Destruktor einfach rollback() aufruft, dann würde das so aussehen:
oos::transaction tr(db); tr.begin(); ostore.insert(new person("George")); ostore.insert(new person("Jane")); ostore.insert(new person("Tim")); ostore.insert(new person("Walter")); tr.commit();
Wenn eine Exception fliegt, wird automatisch gebackrollt. Damit braucht man auch nichts mehr fangen. Nach commit() hat rollback keinen Effekt mehr, natürlich.
Bleibt aber noch dieses begin() und commit(), sollte eine Transaction nicht genau eine "Operation" sein? Mit diesem Interface kann man irgendwie öfter commit() auf einer Transaction aufrufen. Ich würde Folgendes vorschlagen:typedef std::function<void(oos::object_store&)> transaction; db.run_transaction([] (oos::object_store& ostore) { ostore.insert(new person("George")); ostore.insert(new person("Jane")); ostore.insert(new person("Tim")); ostore.insert(new person("Walter")); });
Damit kann man Transactions auch einfach im Voraus erstellen, zurückgeben, kopieren, etc. - Wenn eine Transaction throwt, wird automatisch rollback() aufgerufen.
void run_transaction(transaction t) { try { t(ostore); } catch (...) { rollback(); std::rethrow_exception(std::current_exception()); } }
Na ja wie die Typen dann genau heißen muss man sich noch überlegen, aber ich denke der Grundgedanke ist klar. Danach wird's nur noch interessant was multithreading angeht, uU kann man gleich einen Thread-Pool in das DB-Objekt stecken und dann die Transactions aus einer concurrent_queue abarbeiten oder so, aber da müsste ich etwas länger drüber nachdenken.
-
zussel schrieb:
Sie mal in der Doku, da stehen noch viele andere Dinge: http://zussel.github.io/oos/api/index.html
Ok, offentsichtlich habe ich nicht aufmerksam genug geschaut. Mein erster Eindruck war, dass die Startseite mit dem Beispiel auch schon die ganze Doku war.
zussel schrieb:
Das Ableiten von einer Basisklasse ist wohl oder übel notwendig, damit das Objekt auch korrekt serialisiert werden kann. C++ bietet keine Reflections wie in Java, deshalb finde ich diese Variante gar nicht so schlimm.
Wie würdest Du denn das generische Serialisieren ohne Ableiten realisieren?
Eine Alternative wäre, die benötigten Informationen über externe Klassen, z.B. Traits anzugeben. So habe ich mal ein Persistenz Framework in C++ aufgebaut. War schon einiges an Aufwand, das möglichst generisch zu definieren, und ist auch etwas Tipparbeit, die ganzen Traits für die zu speichernden Objekte zu definieren, aber mir persönlich gefällt der Ansatz besser.
Schau dir vielleicht mal diese Bibliothek an, falls du sie noch nicht kennst:
http://www.qxorm.com/qxorm_en/home.html
Und ein Einführungsbeispiel:
http://www.qxorm.com/qxorm_en/quick_sample.html
Ich kenne die Bibliothek jetzt selber nicht im Detail. Ich bin irgendwann mal drübergestolpert, nachdem ich mein Framework geschrieben hatte. Die haben ein ähnliches Konzept wie ich. Ist zwar alles etwas anders aufgezogen, aber bei denen muss man auch "Klassen" definieren, die den Zugriff auf die Daten beschreiben. Die verwenden zwar wohl Qt, aber es geht auch ohne, ich habs damals nicht verwendet.
zussel schrieb:
Freut mich, dass Dir die API gefällt, vielleicht magst Du sie ja auch mal ausprobieren.
Dafür stört mich der Zwang einer vorgegebenen Klasse leider zu sehr. In meinem Logikcode mag ich sowas eigentlich nicht haben und extra DTO Objekte will ich auch nicht schreiben. Wenn ich aber mal einen Anwendungsfall habe, wo mir das egal ist, werd ich an deine Bibliothek denken.
-
Mechanics schrieb:
Eine Alternative wäre, die benötigten Informationen über externe Klassen, z.B. Traits anzugeben. So habe ich mal ein Persistenz Framework in C++ aufgebaut. War schon einiges an Aufwand, das möglichst generisch zu definieren, und ist auch etwas Tipparbeit, die ganzen Traits für die zu speichernden Objekte zu definieren, aber mir persönlich gefällt der Ansatz besser.
Schau dir vielleicht mal diese Bibliothek an, falls du sie noch nicht kennst:
http://www.qxorm.com/qxorm_en/home.html
Und ein Einführungsbeispiel:
http://www.qxorm.com/qxorm_en/quick_sample.html
Ich kenne die Bibliothek jetzt selber nicht im Detail. Ich bin irgendwann mal drübergestolpert, nachdem ich mein Framework geschrieben hatte. Die haben ein ähnliches Konzept wie ich. Ist zwar alles etwas anders aufgezogen, aber bei denen muss man auch "Klassen" definieren, die den Zugriff auf die Daten beschreiben. Die verwenden zwar wohl Qt, aber es geht auch ohne, ich habs damals nicht verwendet.
QXOrm hatte ich mir auch mal angeschaut. Der QT "Overhead" hat mir dabei nicht so gefallen. Aber den Ansatz, das Serialisieren bzw. die Informationen über die Felder auszulagern, fand ich interessant.
Ich hatte mich damals halt für den anderen Weg entschieden. Nachdem Du mich jetzt nochmal darauf hingewiesen hast, habe ich nochmal darüber nachgedacht.
Momentan wäre es ein ziemlich großer Umbau. Beim Ändern eines Feldes mache ich mir, falls eine Transaktion läuft, ein Backup des Feldes. Das ist mit diesem Ansatz nicht ohne größeren Aufwand zu realisieren. Aber ich werde definitiv diesen Ansatz einmal durchspielen.
Danke für diese Anregung!
Sascha
-
zussel schrieb:
Beim Ändern eines Feldes mache ich mir, falls eine Transaktion läuft, ein Backup des Feldes. Das ist mit diesem Ansatz nicht ohne größeren Aufwand zu realisieren.
Hmm, das versteh ich nicht ganz. Wann musst du irgendwelche Felder ändern? Ich dachte, die Transaktionen laufen über die Datenbank. Wenn ein Datensatz in der Datenbank nicht geändert werden konnte, dann geht es eben nicht und die Transaktion schlägt fehl. An welcher Stelle musst du hier die Instanz der Klasse verändern?
Ich glaube eben, dass der Qt Overhead nicht unbedingt nötig ist. Wie gesagt, ich hab mal was ähnliches geschrieben, ohne Qt. Ich hab aber auch boost::function verwendet, mit C++11 wäre nicht mal das nötig.
Kennst du boost::serialize? Da kann man den Stream Operator auch einfach außerhalb der Klasse implementieren und muss die Klasse nicht verändern.
Was du mit dem Ansatz wahrscheinlich nicht hinbekommst, ist lazy loading. Das könntest du mit deinen eigenen Containern wie object_list wahrscheinlich realisieren. Ohne solche Hilfsmittel wird es in C++ wohl wirklich schwierig.
Du könntest vielleicht zwei Varianten anbieten (klar, ist viel Aufwand und ein größerer Umbau), die jetzige, und eine, bei der man beliebige Klassen speichern kann. Dann könnte man mit deiner Bibliothek ganz normale Klassen speichern (ich glaube, ich bin nicht der einzige, der das wichtig findet), und wenn man dann Gefallen daran gefunden hat und fortgeschrittene Features nutzen möchte, könnte man für einige Klassen DTOs definieren, die dann von deiner Basisklasse erben und die speziellen Container benutzen.
-
Mechanics schrieb:
Hmm, das versteh ich nicht ganz. Wann musst du irgendwelche Felder ändern? Ich dachte, die Transaktionen laufen über die Datenbank. Wenn ein Datensatz in der Datenbank nicht geändert werden konnte, dann geht es eben nicht und die Transaktion schlägt fehl. An welcher Stelle musst du hier die Instanz der Klasse verändern?
Der ObjectStore funktioniert auch In-Memory, also wenn keine Datenbank genutzt wird. Demnach muss ich Transaktionen (commit, rollback) auch im Framework abbilden. Ich habe den ObjektStore ohne eine Datenbankanbindung geplant und umgesetzt (quasi proof of concept).
Mechanics schrieb:
Ich glaube eben, dass der Qt Overhead nicht unbedingt nötig ist. Wie gesagt, ich hab mal was ähnliches geschrieben, ohne Qt. Ich hab aber auch boost::function verwendet, mit C++11 wäre nicht mal das nötig.
Mir ist klar, dass ich den QT Overhead nicht brauche. Wollte damit nur zum Ausdruck bringen, dass ich QT nicht in meinem Framework nutzen möchte.
Mechanics schrieb:
Kennst du boost::serialize? Da kann man den Stream Operator auch einfach außerhalb der Klasse implementieren und muss die Klasse nicht verändern.
Jepp, habe ich mir schon angeschaut. Finde das Konzept recht interessant.
Mechanics schrieb:
Was du mit dem Ansatz wahrscheinlich nicht hinbekommst, ist lazy loading. Das könntest du mit deinen eigenen Containern wie object_list wahrscheinlich realisieren. Ohne solche Hilfsmittel wird es in C++ wohl wirklich schwierig.
Du könntest vielleicht zwei Varianten anbieten (klar, ist viel Aufwand und ein größerer Umbau), die jetzige, und eine, bei der man beliebige Klassen speichern kann. Dann könnte man mit deiner Bibliothek ganz normale Klassen speichern (ich glaube, ich bin nicht der einzige, der das wichtig findet), und wenn man dann Gefallen daran gefunden hat und fortgeschrittene Features nutzen möchte, könnte man für einige Klassen DTOs definieren, die dann von deiner Basisklasse erben und die speziellen Container benutzen.Lazy loading finde ich persönlich interessanter und wichtiger, als zwei Varianten anzubieten. Vielleicht gibt es für jeden Teilaspekt (Serialisierung, Basisklasse, Transkationen, Lazyloading) eine gut Lösung. Dann könnte ich das nach und nach umsetzen.
Ich werde mir mal eine Roadmap überlegen und die "Future Plans" auf der Projektseite dadurch ersetzen.
Gruß
Sascha
-
Moin Zussel,
ich sitze derzeit an einer ähnlichen Geschichte zur (De-)Serialisierung von Objekten.
Das Laden und Speichern von einzelnen Objekten in Datenbanken läuft, es gibt also durchaus auch eine nutzbare Implementierung.
zussel schrieb:
virtual void deserialize(oos::object_reader &rdr)
{
oos::object::deserialize(rdr);
rdr.read("name", name_);
rdr.read("age", age_);
rdr.read("mate", mate_);
}virtual void serialize(oos::object_writer &wrt) const
{
oos::object::serialize(wrt);
wrt.write("name", name_);
wrt.write("age", age_);
wrt.write("mate", mate_);
}Ich habe Deine OOS-Lib noch nicht ausprobiert, mir fiel nur der oben stehende Code ins Auge.
Ich habe die (De-)Serialisierung aus dem Objekt rausgenommen. Das Objekt muss, genau wie bei Dir, von einer Basisklasse abgeleitet sein. Anschließend gebe ich die Memberpointer der Member, die ich serialisieren möchte, in einem Konfigurationsobjekt bekannt. Die Funktionalität zum (De-)Serialisieren ist damit konfigurierbar, ich kann also einen Objekttyp unterschiedlich mappen und ich muss keine (De-)Serialisisungs-Methoden einfügen. Unterschiedliche Konfigurationen können also Teile des Objektes auch in unterschiedlichen Tabellen speichern.
Eigentlich muss ich neben der Ableitung gar nichts einfügen, weil ich die Konfiguration auch außerhalb der Klasse mache. Es sind also keine zusätzlichen Methoden im zu mappenden Objekt erforderlich.zussel schrieb:
So, ich hoffe, ich konnte den einen oder anderen Neugierig machen. Ich würde mich über konstruktive Kritik, Anregungen und natürlich auch Anerkennung freuen.
Ich habe es mir noch nicht groß angeguckt, aber ein Rollback bekomme ich derzeit noch nicht hin. ^^
Aber das ist derzeit auch nicht mein primäres Problem.Sieht aber auf den ersten Blick nicht verkehrt aus. Also schonmal ein anerkennendes Nicken.
-
Moin Xin
Xin schrieb:
ich sitze derzeit an einer ähnlichen Geschichte zur (De-)Serialisierung von Objekten.
Das Laden und Speichern von einzelnen Objekten in Datenbanken läuft, es gibt also durchaus auch eine nutzbare Implementierung.
Irgendwie programmiert man doch sehr oft immer das gleiche - und man freut sich wie ein Schnitzel, wenn der Code dann (auch wieder) funktioniert
Back to topic: Das Laden und Speichern, insbesondere von komplexen, verschachteteln Objekten (und wenn sie dann auch noch Listen/Vektoren enhalten), finde ich schon ziemlich komplex.
Lädst Du die Tabellen nacheinander, oder die Objekte? Falls Du die Tabellen lädst, wie hast Du das Henne ein Problem gelöst, wenn ein Objekt einen anderes enthält dessen Tabelle Du noch nicht geladen hast. Bei mir war das ein guten Stück arbeit.
Xin schrieb:
Ich habe Deine OOS-Lib noch nicht ausprobiert, mir fiel nur der oben stehende Code ins Auge.
Ich habe die (De-)Serialisierung aus dem Objekt rausgenommen. Das Objekt muss, genau wie bei Dir, von einer Basisklasse abgeleitet sein. Anschließend gebe ich die Memberpointer der Member, die ich serialisieren möchte, in einem Konfigurationsobjekt bekannt. Die Funktionalität zum (De-)Serialisieren ist damit konfigurierbar, ich kann also einen Objekttyp unterschiedlich mappen und ich muss keine (De-)Serialisisungs-Methoden einfügen. Unterschiedliche Konfigurationen können also Teile des Objektes auch in unterschiedlichen Tabellen speichern.
Eigentlich muss ich neben der Ableitung gar nichts einfügen, weil ich die Konfiguration auch außerhalb der Klasse mache. Es sind also keine zusätzlichen Methoden im zu mappenden Objekt erforderlich.Ja ich wurde ja auch von einem anderen Poster darauf hingewiesen und Dein Einwand, dass das Serialisieren dadurch konfigurierbar wird (man kann sich sogar verschiedenen Konfigurationen für ein und denselben Objekttyp vostellen) ist nicht von der Hand zu weisen.
Ich denke ich werde das in meiner Roadmap entsprechend einplanen!
zussel schrieb:
So, ich hoffe, ich konnte den einen oder anderen Neugierig machen. Ich würde mich über konstruktive Kritik, Anregungen und natürlich auch Anerkennung freuen.
Xin schrieb:
Ich habe es mir noch nicht groß angeguckt, aber ein Rollback bekomme ich derzeit noch nicht hin. ^^
Aber das ist derzeit auch nicht mein primäres Problem.Commit und Rollback habe ich anhand von Observer- und Visitor-Pattern gelöst. Wenn Du mehr Details haben möchtest, sag Bescheid.
Xin schrieb:
Sieht aber auf den ersten Blick nicht verkehrt aus. Also schonmal ein anerkennendes Nicken.
Vielen Dank und Dir auch weiterhin viel Erfolg bei Deiner Bibliothek (möchtest Du die dann eigentlich auch veröffentlichen)!
Sascha
-
zussel schrieb:
Back to topic: Das Laden und Speichern, insbesondere von komplexen, verschachteteln Objekten (und wenn sie dann auch noch Listen/Vektoren enhalten), finde ich schon ziemlich komplex.
Ich habe zwei Implementationen, die erste lädt Listen und Vektoren, allerdings sind das nicht std::vector und std::list, sondern selbstgeschriebene. Ich arbeite meistens mit selbstgeschriebenen Containern. Die erste wurde allerdings durch den Konstruktor des Objektes konfiguriert, so dass jedes Objekt eine eigene Konfiguration besaß. Das ließ zwar sehr viele Möglichkeiten zur Individuellen Bearbeitung (die keiner braucht), kostet aber Speicherplatz.
Die zweite Implementation ersetzt schließlich die erste, sobald sie alle Fähigkeiten der ersten übernehmen kann.
zussel schrieb:
Lädst Du die Tabellen nacheinander, oder die Objekte?
Ich ziehe die Objekte aus der Tabelle. Die kompletten Tabellen lade ich nicht, sonst bräuchte ich keine Datenbank.
zussel schrieb:
zussel schrieb:
So, ich hoffe, ich konnte den einen oder anderen Neugierig machen. Ich würde mich über konstruktive Kritik, Anregungen und natürlich auch Anerkennung freuen.
Xin schrieb:
Ich habe es mir noch nicht groß angeguckt, aber ein Rollback bekomme ich derzeit noch nicht hin. ^^
Aber das ist derzeit auch nicht mein primäres Problem.Commit und Rollback habe ich anhand von Observer- und Visitor-Pattern gelöst. Wenn Du mehr Details haben möchtest, sag Bescheid.
Ich bin noch nicht sicher, ob ich das zurzeit brauche. YAGNI?
Im Prinzip will ich vergleichsweise einfache Objekte laden und speichern, also wirklich nur Transfer aus der Datenbank und wieder rein. Mir geht es mehr darum, dass ich nicht umständlich die C++-Datenstrukturen auf die Datenbank abbilden muss, sondern einfach meine Datenstrukturen schreibe, konfiguriere und die Datenbank und die ganze sich damit automatisch ergibt.
zussel schrieb:
Xin schrieb:
Sieht aber auf den ersten Blick nicht verkehrt aus. Also schonmal ein anerkennendes Nicken.
Vielen Dank und Dir auch weiterhin viel Erfolg bei Deiner Bibliothek (möchtest Du die dann eigentlich auch veröffentlichen)!
Meine "Bibliothek" besteht inzwischen aus einer Reihe von Libs. Die Datenbankgeschichte ist eher eine Randnotiz der IO-Lib.
Über eine Veröffentlichung denke ich nach, aber dafür müsste ich da noch einiges erst wieder rauspulen und ändern, da ich wie gesagt, nicht unbedingt mit Standard-Containern unterwegs bin. Das ganze ist also abhängig von anderen Libs von mir.zussel schrieb:
Sascha
Auch so
-
zussel schrieb:
Mein nächster Ansatz würde in die Richtung RAII gehen. Dann würde das ungefähr so aussehen:
{ oos::transaction tr(db); // beim anlegen der Transaktion wird automatisch ein begin() aufgerfufen ostore.insert(new person("George")); ostore.insert(new person("Jane")); ostore.insert(new person("Tim")); ostore.insert(new person("Walter")); // wird der Scope verlassen wird beim zerstören der Transaktion // automatisch commit() und je nachdem ein rollback() aufgerufen. }
Das klappt so nicht. Siehe http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3614.pdf.
Wie wäre es mit:
#define OOS_TRANSACTION \ for(bool _once = true; _once; _once = false) \ for(oos::transaction _transaction; _once; _once = false, _transaction.commit())
oos::transaction muss dann im Dtor ein rollback machen, sofern nicht commitet wurde.
OOS_TRANSACTION { ostore.insert(new person("George")); if(rand() % 42 == 0) break; // nope, transaktion abbrechen }
-
***** Es gibt 'ne neue Version, siehe ersten Post *****
-
Ist leider unbenutzbar, weil nicht exception safe. Da sind überall Leaks und wer weiß was noch alles.
-
TyRoXx schrieb:
Ist leider unbenutzbar, weil nicht exception safe. Da sind überall Leaks und wer weiß was noch alles.
Hallo TyRoXx,
danke für die Rückmeldung. Die Aussagen sind mir aber etwas Pauschal.
Warum ist Sie unbenutzbar? Eine Bibliothek muss nicht unbedingt exception safe sein (ist aber ein noch nicht erreichtes Ziel). Welche Stellen sind Dir denn aufgefallen?
Wo sind die Memory Leaks? Alle Unit Test mit valgrind sagen mir das Gegenteil. Was hast Du ausprobiert und wo trat das Problem auf?
Was willst Du mit "was weiß ich noch alles" sagen?
Mein Ziel ist eine saubere, leicht zu benutzende Bibliothek, daher ist es für mich hilfreicher, wenn auf konkrete Probleme gezeigt wird. Daran kann ich arbeiten.
Könntest Du Deine Kritik deshalb etwas konkretisieren?
-
zussel schrieb:
Eine Bibliothek muss nicht unbedingt exception safe sein (ist aber ein noch nicht erreichtes Ziel).
Muss sie nicht. Aber wenn sie es nicht ist, wird kein vernünftiger Programmierer sie verwenden.
zussel schrieb:
Wo sind die Memory Leaks? Alle Unit Test mit valgrind sagen mir das Gegenteil. Was hast Du ausprobiert und wo trat das Problem auf?
So gut wie jedes
new
in deinem Code wird zu einem Leck sobald eine Ausnahme fliegt und das manuelledelete
verhindert. Du wirfst selbst Ausnahmen, also geht es nicht nur um hypothetischebad_alloc
s.zussel schrieb:
Was willst Du mit "was weiß ich noch alles" sagen?
Wenn du es nicht hinbekommst alle Ressourcen wieder freizugeben, sind da vielleicht noch schlimmere Fehler. Vielleicht kann eine Ausnahme an einer bestimmten Stelle eins deiner Objekte in einem ungültigen Zustand zurücklassen. So ein Zustand könnte von einem bösartigen Angreifer ausgenutzt werden.
zussel schrieb:
Mein Ziel ist eine saubere, leicht zu benutzende Bibliothek, daher ist es für mich hilfreicher, wenn auf konkrete Probleme gezeigt wird. Daran kann ich arbeiten.
Könntest Du Deine Kritik deshalb etwas konkretisieren?
Hier ist diverse Kritik, die mir spontan einfällt:
class OOS_API transaction : public object_observer { [...] private: static long id_counter; }; [...] id_ = ++transaction::id_counter;
Diese globale Variable könnte in mehreren Threads verwendet werden.
Deine Transaktionen sind also nicht Thread-safe.void sql::append(const condition &c) { field_ptr f(new field(c.column().c_str(), c.type(), host_field_vector_.size(), true)); /*std::list<token*>*/ token_list_.push_back(new condition_token(c)); //leak host_field_map_.insert(std::make_pair(c.column(), f)); host_field_vector_.push_back(f); }
(Die Kommentare sind von mir.)
Wenn das erstepush_back
fehlschlägt, geht das übergebene Objekt verloren.
Außerdem sehe ich in der Datei keinen Grund fürstd::list
, den ineffizientesten Container in C++.table::~table() { if (insert_) { delete insert_; delete update_; delete delete_; delete select_; } }
Ich frage mich, ob das
if
da irgendeinen Zweck erfüllt oder ob es wirklich redundant ist. Es könnte ja sein, dassupdate_
etc nicht gelöscht werden sollen, fallsinsert_==nullptr
. Ganz abgesehen, dassdelete
immer eine schlechte Idee ist.if (mysql_real_connect(&mysql_, host.c_str(), user.c_str(), (has_pwd ? passwd.c_str() : 0), db.c_str(), 0, NULL, 0) == NULL) { printf("Connection Error %u: %s\n", mysql_errno(&mysql_), mysql_error(&mysql_)); exit(1); }
Das ist keine sinnvolle Fehlerbehandlung.
MYSQL* mysql_database::operator()() { return &mysql_; }
Aus verschiedenen Gründen ist das eine schlechte Idee. Bitte selber recherchieren.
info_[index].buffer = new char[sizeof(long)]; memset(info_[index].buffer, 0, sizeof(long)); // das gleiche wie: info_[index].buffer = new char[sizeof(long)]();
Wenn schon
new[]
, dann wenigstens richtig.session::~session() { if (impl_) { if (type_ == "memory") { delete impl_; } else { database_factory::instance().destroy(type_, impl_); } } }
Das sieht unnötig kompliziert aus.
void session::push_transaction(transaction *tr) { if (!transaction_stack_.empty()) { // cout << "unregister transaction observer [" << transaction_stack_.top() << "]\n"; ostore_.unregister_observer(transaction_stack_.top()); } transaction_stack_.push(tr); // cout << "register transaction observer [" << tr << "]\n"; ostore_.register_observer(tr); }
Man sieht sehr oft dieses
cout
-Debugging. Das erhöht nicht gerade mein Vertrauen in die Korrektheit des Programms. Wenn du etwas mal nicht sinnvoll mit einem Test abdecken kannst, nimm einen Debugger./** * Returns the classname of the object * * @return Name of the object type */ const char* classname() const; [...] const char* object::classname() const { if (proxy_ && proxy_->node) { return proxy_->node->type.c_str(); // return proxy_->node->producer->classname(); } else { return 0; } }
Die Dokumentation ist nicht nur wortkarg und wenig nützlich. Sie lügt mich sogar an.
nullptr
ist für mich kein "Name". Man hätte hier deutlich hinschreiben müssen, dassnullptr
zurückkommen kann. Oder man hätte einfachconst std::string *
zurückgeben können.class OOS_API object : public object_atomizable { public: [...] /** * Modify the char attribute assigning * the new given char value to attributes * reference. * * @param attr Pointer to character string to change. * @param max_size The maximum capacity of the destinition array. * @param val New stringto set. * @param size The length of the new string. * @throw std::logic_error. */ void modify(char *attr, int max_size, const char *val, int size) { if (max_size < size) { throw std::logic_error("not enough character size"); } mark_modified(); #ifdef WIN32 strcpy_s(attr, max_size, val); #else strcpy(attr, val); #endif }
Ich habe nicht den blassesten Schimmer wozu das gut sein soll.
Das einzige was die Methode mit demobject
zu tun hat, ist der Aufruf vonmark_modified
. Was soll der Rest da drin? Warumstrcpy
? Warumint
stattsize_t
?
EDIT: Ah, die Methode wird im Beispiel verwendet. Ist als Syntactic Sugar nicht so schlecht.#ifdef WIN32 #ifdef oos_EXPORTS #define OOS_API __declspec(dllexport) #define EXPIMP_TEMPLATE #else #define OOS_API __declspec(dllimport) #define EXPIMP_TEMPLATE extern #endif #pragma warning(disable: 4251) #else #define OOS_API #endif
Das findet sich in vielen Headern wieder. Hätte man das nicht in einem zentralen Header unterbringen können?
template < typename T > bool assign(const T &val) { delete [] data_; data_ = new char[sizeof(T)]; memcpy(data_, &val, sizeof(T)); size_ = sizeof(T); return false; }
Das ist ein Beispiel für so eine typische Sicherheitslücke, von denen man immer liest. Wenn das
new
fehlschlägt, zeigtdata_
auf bereits freigegebenen Speicher. Wenn du Pech hast, kann ein guter Hacker das beliebig ausnutzen. In diesem Fall dürfte es wegen der festen und vermutlich geringen Größe der Allokation schwierig sein dasbad_alloc
herbeizuführen, aber es könnte problematischere Vorkommen dieses Fehlers in deinem Programm geben.
Unschön ist außerdem, dass da drei malsizeof(T)
steht statt ein malsizeof(val)
. Solche Redundanz ist völlig unnötig.
Warum baust du überhauptstd::vector
nach?Es gibt mehrere leere Dateien, zum Beispiel
src/object/attribute_serializer.cpp
. Es sind unbeabsichtigte Dateien eingecheckt, zum Beispielsrc/tools/byte_buffer.o
oder diverseMakefile
s.
-
Halllo TyRoXx,
das nenn' ich doch mal konkret! Offensichtlich habe ich mich mehr auf die Funktionalität konzentriert als auf die Sicherheit. Daher werde ich im nächsten Release hauptsächlich alle Unzulänglichkeiten und Sicherheitslücken entfernen.
Du bist wahrscheinlich der erste, der sich meinen Code mal genauer angesehen hat. Dafür ein dickes Dankeschön ! Und ich hoffe, dass Du im nächsten Release so gut wie nichts mehr finden wirst.
Beste Grüße
zussel
-
Und bitte nicht einfach durch den Code gehen und versuchen die ganzen Leaks mit 30 try-catch Blocks zu entfernen, sondern RAII nutzen. Überall da, wo in deinem Code eine Ressource außerhalb eines Konstruktors angefordert wird, oder außerhalb eines Destruktors freigegeben wird, hast du ein Problem. Du hast in vielen Fällen sogar schon ein Problem wenn du mehrere solcher Ressourcen auf einmal anforderst. Am besten du hälst dich an diese Regel: Ein Objekt (mit Konstruktor und Destruktor) pro Ressource. Dann kann wenig schief gehen. Und mache dir gedanken über Datentypen. int size? Kann size wirklich negativ sein? Dann bitte unsigned. Ist unsigned wirklich immer groß genug? (64-bit System?) Vielleicht doch lieber std::size_t. printf will niemand sehen in einer Bibliothek, genau so wenig wie exit. Beides ist tabu, nur Rückgabewerte und Exceptions sind erlaubt. (Oder Parameter.)
-
Hallo cooky451
Auch Dir danke für die Hinweise! Diese werde ich auch beherzigen. Ich hoffe, ich kann meine Bibliothek damit schnell stabil machen. Einen guten Funktionsumfang hat sie in meinen Augen schon.
Gruß
zussel
-
***** Es gibt 'n Bugfix Release, siehe ersten Post *****