4. Neue C++ ORM Bibliothek "oos" v0.2.1

Neue C++ ORM Bibliothek "oos" v0.2.1


  • 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 😉

    0
  • K

    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
}
    0
  • Z

    ***** Es gibt 'ne neue Version, siehe ersten Post *****

    0
  • T

    Ist leider unbenutzbar, weil nicht exception safe. Da sind überall Leaks und wer weiß was noch alles.

    0
  • Z

    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?

    0
  • T

    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 manuelle delete verhindert. Du wirfst selbst Ausnahmen, also geht es nicht nur um hypothetische bad_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 erste push_back fehlschlägt, geht das übergebene Objekt verloren.
    Außerdem sehe ich in der Datei keinen Grund für std::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, dass update_ etc nicht gelöscht werden sollen, falls insert_==nullptr . Ganz abgesehen, dass delete 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, dass nullptr zurückkommen kann. Oder man hätte einfach const 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 dem object zu tun hat, ist der Aufruf von mark_modified . Was soll der Rest da drin? Warum strcpy ? Warum int statt size_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, zeigt data_ 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 das bad_alloc herbeizuführen, aber es könnte problematischere Vorkommen dieses Fehlers in deinem Programm geben.
    Unschön ist außerdem, dass da drei mal sizeof(T) steht statt ein mal sizeof(val) . Solche Redundanz ist völlig unnötig.
    Warum baust du überhaupt std::vector nach?

    Es gibt mehrere leere Dateien, zum Beispiel src/object/attribute_serializer.cpp . Es sind unbeabsichtigte Dateien eingecheckt, zum Beispiel src/tools/byte_buffer.o oder diverse Makefile s.

    0
  • Z

    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

    0
  • C

    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.)

    0
  • Z

    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

    0
  • Z

    ***** Es gibt 'n Bugfix Release, siehe ersten Post *****

    0
Anmelden zum Antworten