IdHTTP Automatisches Programmupdate

[Zero01]

Hallo zusammen,
ich habe mich für einen Updatemechanismus mit einer separaten Executable entschieden. Das Hauptprogramm prüft zunächst alle 24 Stunden ob es ein Update gibt und überprüft vor dem Start des Updateprozesses ob es für selbigen eine Aktualisierung gibt. Würde eine Update für den Updateprozess gefunden wird dieser zunächst heruntergeladen, aktualisiert und dann gestartet.

Da dieses Projekt noch relativ Jung ist weiß man nicht ob die Updateroutinen früher oder später Ausgeweitet werden müssen und daher soll diese eben auch aktualisierbar sein.

Die Frage ist auch ob es designtechnisch nicht wohl besser ist dieses in einen TThread auszulagern.

Findet ihr in dem Quelltext noch grobe Schnitzer womit man böse auf die Nase fallen könnte?

//------------------------------------------------------------------------
AnsiString __fastcall TForm1::GetApplicationVersion(AnsiString path)
{

	 AnsiString file_version;
	 DWORD handle;
	 DWORD size;
	 bool status;

	 if(FileExists(path))
	 {

			size = GetFileVersionInfoSize(path.c_str(), &handle);
			if(size == 0)
			return ("");

			char *buffer = new char [size];
			status = GetFileVersionInfo(path.c_str(), 0, size, buffer);
			if(status == false)
			{
				delete [] buffer;
				return ("");
			}

		   UINT datasize;
		   unsigned short *translation;

		   status = VerQueryValue( buffer, "\\VarFileInfo\\Translation",(void **) &translation, &datasize);
		   if(status == false)
		   {
			   delete [] buffer;
			   return ("");
		   }

		   AnsiString prefix = "\\StringFileInfo\\" + AnsiString::IntToHex(translation [0], 4)
							   + AnsiString::IntToHex(translation [1], 4);

		   file_version = GetVersionKey(buffer, prefix, "FileVersion");
		   if(file_version != "")
		  {
		   delete [] buffer;
		   return file_version;
		  }

	 }

	return ("");

}
//---------------------------------------------------------------------------
char* TForm1::GetVersionKey(char *buffer, const AnsiString &prefix, char *key)
{
  char *versiontext;
  unsigned int versiontext_size;

  AnsiString fullkey = prefix + "\\" + key;

  bool status = VerQueryValue(buffer, fullkey.c_str(),(void **) &versiontext, &versiontext_size);

  if(status == true)
	return(versiontext);
  else
    return("");
}
//---------------------------------------------------------------------------
void __fastcall TForm1::IdHTTP1Work(TObject *ASender, TWorkMode AWorkMode, __int64 AWorkCount)
{
   if(UpdateProcedureWorking == true)
   StatusBar1->Panels->Items[2]->Text = " Auto Update: " + String(AWorkCount) + " bytes heruntergeladen";

   Application->ProcessMessages();
}
//---------------------------------------------------------------------------
void __fastcall TForm1::IdHTTP1WorkBegin(TObject *ASender, TWorkMode AWorkMode, __int64 AWorkCountMax)
{

	 if(AWorkCountMax > 0)
	 DownloadFileSize = AWorkCountMax;

}
//---------------------------------------------------------------------------
void __fastcall TForm1::UpdateTimerTimer(TObject *Sender)
{

	 try
	 {
			AnsiString WebUpdaterVersion = "";
			AnsiString WebUpdateMainVersion = "";

			AnsiString MainAppPath = AnsiString(CurrentDirectory) + "\\project1.exe";
			AnsiString UpdateAppPath = AnsiString(CurrentDirectory) + "\\Update_project1.exe";

			WebUpdaterVersion = IdHTTP1->Get("http://www.xxx.de/updater_version.txt");
			WebUpdateMainVersion = IdHTTP1->Get("http://www.xxxx.de/xxxx/main_version.txt");
			IdHTTP1->Disconnect();

			if(WebUpdaterVersion > GetApplicationVersion(UpdateAppPath) && WebUpdaterVersion != "" && GetApplicationVersion(UpdateAppPath) != "")
			{

				   if(Update_UpdateApplication() == true)
				  {

					 if(WebUpdateMainVersion > GetApplicationVersion(MainAppPath) && WebUpdateMainVersion != "" && GetApplicationVersion(MainAppPath) != ""
					 && FileExists(UpdateAppPath) == true)
					 ShellExecute(NULL, "open", UpdateAppPath.c_str(), NULL, NULL, SW_SHOW);
				  }

			}
			else
			{
                if(WebUpdateMainVersion > GetApplicationVersion(MainAppPath) && WebUpdateMainVersion != "" && GetApplicationVersion(MainAppPath) != ""
				&& FileExists(UpdateAppPath) == true)
				ShellExecute(NULL, "open", UpdateAppPath.c_str(), NULL, NULL, SW_SHOW);

            }

	 }
	 catch(Exception &ex){}

}
//---------------------------------------------------------------------------
//---------------------------------------------------------------------------
bool __fastcall TForm1::Update_UpdateApplication()
{

	  TMemoryStream *downstream = new TMemoryStream();

	  try
	 {
		 AnsiString oldmain_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe";
		 AnsiString update_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe.upd";

		 AnsiString VersionText = IdHTTP1->Get("http://www.xxxx.de/xxxx/update_version.txt");
		 IdHTTP1->Disconnect();

		 if(VersionText > CurrentVersion)
		{
            UpdateProcedureWorking = true;
			IdHTTP1->Get("http://www.xxxx.de/xxxx/Release/xxxx_Update.exe", downstream);
			IdHTTP1->Disconnect();

			  if(downstream->Size == DownloadFileSize && downstream->Size > 0 && DownloadFileSize > 0)
			  {
					 downstream->SaveToFile(update_exe);

					 if(FileExists(update_exe) && GetFileSize(update_exe) == DownloadFileSize)
					 {

							if(remove(oldmain_exe.c_str()) == 0 )
							{
								   if(rename(update_exe.c_str() , oldmain_exe.c_str()) == 0)
								  {
									  UpdateProcedureWorking = false;
									  delete downstream;
									  return true;
								  }
								  else
								  {
									  if(FileExists(update_exe) == true)
									  remove(update_exe.c_str());

									  UpdateProcedureWorking = false;
									  delete downstream;
									  return false;
								  }
							}
							else
							{
								if(FileExists(update_exe) == true)
								remove(update_exe.c_str());

								UpdateProcedureWorking = false;
								delete downstream;
								return false;
							}

					 }
					 else
					 {
						 if(FileExists(update_exe) == true)
						 remove(update_exe.c_str());

						 UpdateProcedureWorking = false;
						 delete downstream;
						 return false;
					 }
			  }
		}
	}
	catch (Exception& ex){
	StatusBar1->Panels->Items[2]->Text = " Update Error: " + ex.ToString();}

	UpdateProcedureWorking = false;
	delete downstream;
	return false;
}
//---------------------------------------------------------------------------
__int64 __fastcall TForm1::GetFileSize(AnsiString &path)
{
                  if(FileExists(path) == true)
                 {
			ifstream file;
			file.open(path.c_str(), ios::in);

			if(file.is_open())
		   {
			 file.seekg(0,ios::end);
			 __int64 size = file.tellg();

			 file.close();
			 return size;
		   }
                 }

	  return 0;
}

[Zero01]

Nach und nach habe ich die Neuerungen in den ersten Post gepackt.

[audacia]

Ich äußere mal alles, was mir dazu einfällt:

Zunächst stehe ich dem ganzen Vorhaben skeptisch gegenüber, weil automatische Updater nur selten wirklich benötigt werden. Allgemein würde ich davon Abstand nehmen, zufriedenen Benutzern ein Update aufzuzwingen. Denn das machen eh schon viel zu viele Programme; es nervt mich im Alltag tierisch, wenn mein CD-Brennprogramm, mein Bildbetrachter, das PDF-Printing-Tool etc. sich einbilden, ich müßte von ihnen immer die aktuellste Version haben. Natürlich kann es sein, daß es bei deinem Programm aus irgendeinem Grund erforderlich ist, daß es so intensiv aktualisiert werden muß, und wenn alle Benutzer darauf vorbereitet sind, ist das ja in Ordnung. Aber erwäge, ob es wirklich sein muß, und mache es wenigstens optional.

Außerdem solltest du bedenken, daß evtl. nicht alle deine Benutzer über Adminrechte verfügen. Ich darf mich zu dem Thema kurz selbst zitieren:

ich selbst schrieb:

Ein gutes Beispiel dafür, wie man Auto-Updater nicht macht, findest du bei Oracle. Wann immer ich an einem Firmen- oder Uni-PC mit eingeschränkten Rechten saß, auf dem die JRE installiert war, poppte nach ein paar Minuten der Java-Updater auf und forderte mich auf, ein Update zu installieren, obwohl ich es weder brauchte noch die erforderlichen Rechte hatte. Und ich habe die Vermutung, daß Java die automatischen Updates von Zeit zu Zeit wieder aktiviert, obwohl ich sie ausgestellt und/oder den Autostart-Eintrag entfernt habe. Das und die Drive-by-Toolbar in der JRE geben der Reputation von Java echt den Rest.

(Quelle ist dieser Thread, denn du allgemein mal lesen solltest.)

Abgesehen davon sind Auto-Updater immer eine Sicherheitslücke, sofern sie nicht einen kryptographischen Signaturmechanismus verwenden. Mit jedem Programm, das gefragt oder ungefragt neue Binaries von irgendeinem Server aus dem Netz zieht und sie installiert, wächst der Angriffsvektor; es braucht nur einer der Server mit Malware infiziert zu sein, um dir bösartige Software unterzuschieben, ohne daß du es merkst. Auch ein man-in-the-middle attack ist nicht schwer, wenn du nicht darauf achtest, daß die Verbindung zum Server durch SSL geschützt ist. (Selbst mit aktiver SSL-Verschlüsselung kann dein Provider die Gelegenheit nutzen, um dir den Staatstrojaner unterschieben, wenn das dein LKA veranlaßt hat, aber das Szenario lassen wir mal außer acht ) Wenn du deinen Updater also ordentlich machen willst, dann solltest du deine Update-Pakete irgendwie signieren und im Auto-Updater auf eine gültige Signatur prüfen. Am besten verwendest du dazu GnuPG, das kostet nichts und man muß sich nicht mit Zertifikaten herumärgern.

Jetzt noch ein wenig Detailkritik:

Zero01 schrieb:

Die Frage ist auch ob es designtechnisch nicht wohl besser ist dieses in einen TThread auszulagern.

Doch, wäre es. Aber lange nicht so wichtig wie eine ordentliche Signaturprüfung.

Zero01 schrieb:

AnsiString __fastcall TForm1::GetApplicationVersion(AnsiString path) [...]
char* TForm1::GetVersionKey(char *buffer, const AnsiString &prefix, char *key) [...]
__int64 __fastcall TForm1::GetFileSize(AnsiString &path) [...]

Warum sind das Memberfunktionen von TForm1? Könnten doch ebenso freie Funktionen sein, oder?

Aus folgender Funktion habe ich mal alle interessanten Teile entfernt. Fällt dir irgendwas auf?

Zero01 schrieb:

bool __fastcall TForm1::Update_UpdateApplication()
{
     [...]
     try
     {
         [...]
        {
            if([...])
              {
                     if([...])
                     {
                            if([...])
                            {
                                  if([...])
                                  {
                                      UpdateProcedureWorking = false;
                                      delete downstream;
                                      return true;
                                  }
                                  else
                                  {
                                      [...]
                                      UpdateProcedureWorking = false;
                                      delete downstream;
                                      return false;
                                  }
                            }
                            else
                            {
                                [...]
                                UpdateProcedureWorking = false;
                                delete downstream;
                                return false;
                            }
                     }
                     else
                     {
                         [...]
                         UpdateProcedureWorking = false;
                         delete downstream;
                         return false;
                     }
              }
        }
    }
    [...]

    UpdateProcedureWorking = false;
    delete downstream;
    return false;
}

Schau dir mal an, wie try / __finally funktioniert, das erspart dir viel Schreibarbeit. Und mittelfristig solltest du dich informieren, was RAII ist und wie man es benutzt.

Das hier macht man auch nicht:

Zero01 schrieb:

if(UpdateProcedureWorking == true) [...]
if(FileExists(path) == true) [...]

Was denkst denn du, wozu der Datentyp bool gut ist? Laß den Vergleich weg, dann liest es sich auch viel natürlicher:

if(UpdateProcedureWorking) [...]
if(FileExists(path)) [...]

Auch das macht man nicht wegen der Gefahr eines Stack-Overflow:

Zero01 schrieb:

Application->ProcessMessages();

Fällt aber sicher eh raus, wenn du den Download mal asynchron gemacht hast.

Statt AnsiString verwende einfach durchweg String , dann hast du weniger Probleme mit Unicode-Zeichen in Dateinamen, wenn du C++Builder 2009 oder neuer benutzt. Die Stringliterale mußt du dann auch anpassen, aber das merkst du dann schon.

Deine Funktion GetFileSize() gefällt mir außerdem inhaltlich nicht, die ist sehr umständlich. Rufe einfach die Win32-Funktion GetFileAttributesEx () auf, die macht die Arbeit für dich und ist ein, zwei Größenordnungen schneller. Überhaupt würde ich nicht Win32- und C-RTL-Funktionen durcheinanderwerfen. Benutze lieber DeleteFile() statt remove() , dann klappt das später auch mit Unicode.

[Zero01]

Hallo Audacia,
vielen lieben Dank für deine Ausführungen. Das Hauptprojekt ist eine Kiosksoftware für unsere Mitarbeiter. Alle bisher verfügbaren Kiosksysteme der einschlägigen Hersteller haben unserem Team aufgrund diverser Details im Bezug zu dem was gefordert ist nicht gefallen. Das jetzige selbstgeschriebene Kiosksystem konnten wir so designen das es passt. Jetzt kommt der Haken mit dem Autoupdate. Es gibt Stationen (ohne jeglichen IT-Support vor Ort) in München, Stuttgart, Frankfurt, Düsseldorf.....etc die aus einer Quelle herraus aktualisierbar sein sollen falls Funktionen im Laufe der Zeit hinzugefügt werden müssen. Unsere Geschäftsführung ist da sehr kurzfristig enorm kreativ. Ich habe da wirklich kein Interesse durch ganz Deutschland zu fahren um ein Patch einzuspielen. Die Kiosksysteme an sich sind standalone Systeme die mittels Benutzerrichtlinienobjekt auf eine Minimalfunktion heruntergebrochen sind und jeweils an einem DSL Router hängen.

Eine Userinteraktion falls es ein Update gibt war bisher bei anderen Projekten immer Standard. Aber bei einem Kiosksystem gibts da ein Problem. Welche Person soll dies zuverlässig bestätigen? Frau Musterfrau zwischen Pause, Butterbrot und nen kurzen Blick in die FAZ Online, die keinerlei IT-Bezug hat, sicher nicht. Die einzige Lösung die mir bisher einfällt ist ein Hinweiß das in x Minuten eine Aktualisierung erfolgt und diese dann koste es was es wolle ausgeführt wird.

Der Webserver der über die Updateverzeichnisse verfügt befindet sich in unserem Serverraum gleich neben meinen Oracle Racks Deine Sicherheitsaspekte stimmen mich äusserst nachdenklich.

Die Entwicklungsumgebung ist eine frisch gekaufte kommerzielle Version des C++ Builder XE6 Pro. Kommen wir jetzt zu deinem Input worüber ich mich sehr gefreut habe. Vielen Dank nochmals an dieser Stelle!

Konstrukte wie

if(boolean == true)

sind natürlich Käse. Wird vereinfacht....

In Zeiten meiner Ausbildung wurde immer darauf gepocht klassenspezifisch Memeberfunktionen zu integrieren. Hier habe ich noch nicht über den Tellerrand geschaut und die Klasse TForm1 verwurstet. Ich habe mir ehrlich gesagt noch nicht die Frage gestellt ob hier weitere Klassen Sinn machen und verstehe deine Anmerkung zu RAII. So könnte ich z.B. Dateioperationen oder Initialisierung von Objects direkt in den Klassenkonstruktor packen und die Bordmittel via Destruktor automatisch nach verlassen des Namensbereiches freigeben. Schon oft gemacht nur hier nicht angewendet. Würde man alles so in einer sepparaten Klasse definieren, mit try __finally die Main() Funktion durchlaufen lassen und jeglichen Fehler durch nur eine Fehlerbehandlung abarbeiten, hätte man alles in klare definierte logische Abschnitte verpackt ohne dieses if else gewusel, das meinst du doch, oder?

Könnte man vor Aufruf des Konstruktors MeineKlasse klassenkonstruktor(object,var,var...etc) z.B einen boolean als Referenz übergeben, das Ergebnis des Routinendurchlaufs anhand der Referenzierung in die zuvor deklarierte Variable packen um nach Aufruf des Destruktors entsprechend im "Hauptprogramm" reagieren zu können?

Ok,

Application->ProcessMessages();

fliegt aufgrund der Gefahr des Stack Overflows raus. Den negativen Effekt bei einem asynchronen Download resultierte in einem teils eingefrohrenen Mainthread bedingt durch einen Stack Overflow. Richtig?

Die Verwendung von GetFileAttributesEx () sowie DeleteFile() ist ein super Hinweiß. Ich habe leider noch keine modernere Funktion zu rename() gefunden. Gefühlt stammt diese genauso aus der C-RTL wie delete().

Dateinamenstechnisch passt allerdings AnsiString da ich keinerlei Länderspezifische Umlaute oder Sonderzeichen nutze. Klar, als Unicode String universeller und kann auf Literale mit entsprechenden Konvertierungsmechanismen reagieren.

Übermorgen bin ich wieder im Büro und werde Hausaufgaben machen und danach das Ergebnis posten. Nochmals herzlichen Dank!

Und GnuPG werde ich mir genau ansehen

[audacia]

Zero01 schrieb:

Eine Userinteraktion falls es ein Update gibt war bisher bei anderen Projekten immer Standard. Aber bei einem Kiosksystem gibts da ein Problem. Welche Person soll dies zuverlässig bestätigen? Frau Musterfrau zwischen Pause, Butterbrot und nen kurzen Blick in die FAZ Online, die keinerlei IT-Bezug hat, sicher nicht.

Die richtige Lösung dafür wäre der Restart Manager. Dein Programm muß dazu in der Lage sein, den aktuellen Zustand zu persistieren und beim nächsten Start wiederherzustellen, so daß es in jedem Moment ohne Datenverlust beendet und neugestartet werden kann. Dann erstellst du einen Installer, der den Restart Manager benutzt, um dein Programm vor dem Update zu beenden und hinterher wieder zu starten (z.B. mit InnoSetup). Schließlich erstellst du mit dem Windows-Taskplaner eine Aufgabe, die einmal täglich ausgeführt wird und deinen Auto-Updater anstößt, der überprüft, ob es eine neue Version gibt, und diese ggf. installiert.

Zero01 schrieb:

In Zeiten meiner Ausbildung wurde immer darauf gepocht klassenspezifisch Memeberfunktionen zu integrieren. Hier habe ich noch nicht über den Tellerrand geschaut und die Klasse TForm1 verwurstet. Ich habe mir ehrlich gesagt noch nicht die Frage gestellt ob hier weitere Klassen Sinn machen

Das ist sicher Geschmackssache, aber wenn eine Funktion wie z.B. GetFileSize() oder GetApplicationVersion() nur einen einzigen Zweck erfüllt und keinen Kontext benötigt (außer halt dem globalen Anwendungskontext), dann gehört sie meiner Ansicht nach nicht in eine Klasse. Höchstens in einen separaten Namespace.

Zero01 schrieb:

und verstehe deine Anmerkung zu RAII. So könnte ich z.B. Dateioperationen oder Initialisierung von Objects direkt in den Klassenkonstruktor packen und die Bordmittel via Destruktor automatisch nach verlassen des Namensbereiches freigeben. Schon oft gemacht nur hier nicht angewendet.

Genau so. Wunderbar, wenn du das schon kennst, ist es ja nur eine Fingerübung.

Zero01 schrieb:

Würde man alles so in einer sepparaten Klasse definieren, mit try __finally die Main() Funktion durchlaufen lassen und jeglichen Fehler durch nur eine Fehlerbehandlung abarbeiten

Moment. try / __finally und try / catch ist zweierlei, und du scheinst letzteres zu meinen. try / __finally funktioniert etwa wie RAII, nur eben explizit. Es geht also nicht nur um Fehlerbehandlung, du kannst auch das machen:

TMemoryStream* myStream = ...;
try
{
    if (...)
    {
        ...
        return true;
    }
    else
    {
        ...
        return false;
    }
}
__finally
{
    delete myStream;
}

myStream wird auf jeden Fall freigegeben, weil der __finally -Abschnitt immer ausgeführt wird, egal auf welchem Weg die Funktion verlassen wird. Für einen Zeiger würde man natürlich besser einen Smartpointer verwenden, aber für UpdateProcedureWorking = false; finde ich try / __finally sogar besser, weil es leichter zu lesen ist. Für einen RAII-Ansatz müßte man sich extra ein UpdateProcedureStatusGuard -Objekt definieren, das kommt mir immer etwas sperrig vor.

Zero01 schrieb:

Könnte man vor Aufruf des Konstruktors MeineKlasse klassenkonstruktor(object,var,var...etc) z.B einen boolean als Referenz übergeben, das Ergebnis des Routinendurchlaufs anhand der Referenzierung in die zuvor deklarierte Variable packen um nach Aufruf des Destruktors entsprechend im "Hauptprogramm" reagieren zu können?

Versteh ich nicht.

Zero01 schrieb:

Den negativen Effekt bei einem asynchronen Download resultierte in einem teils eingefrohrenen Mainthread bedingt durch einen Stack Overflow. Richtig?

Nein. Schau mal nach, was ein Stack-Overflow ist; da stürzt üblicherweise dein Programm ab. Ich schrieb nur, daß beim Aufruf von TApplication::ProcessMessages() grundsätzlich diese Gefahr besteht, nicht, daß das bei dir auch passiert. Was du beobachtest, passiert halt, wenn man längere Operationen wie Datei-Downloads im UI-Thread vornimmt: der Thread arbeitet oder wartet auf irgendetwas, und deshalb kann das Fenster nicht auf Benutzereingaben reagieren. Deshalb nimmt man dafür üblicherweise einen Hintergrundthread.

Zero01 schrieb:

Ich habe leider noch keine modernere Funktion zu rename() gefunden.

MoveFile() . Modernität ist außerdem nicht das richtige Kriterium (ist beides völlig archaisch), eher Konsistenz.

[Zero01]

Hallo Audacia,
ich habe angefangen deinen Input zu verarbeiten und mache mit der Hauptupdateroutine den Anfang. Der Übung halber setze ich erstmal mein ursprüngliches Vorhaben in die Praxis um und dann werde ich nach und nach deine Inputs bezüglich der Dateiverschlüsselung und des Installers implementieren. Das ist wirklich eine tolle Übung für mich und mache jetzt mal einen Anfang:

try __finally ist wirklich eine tolle Sache, nur hat mir das Exception handling gefehlt. Die Embarcadero Docs empfehlen folgende Vorgehensweise

bool __fastcall TForm1::Update_UpdateApplication()
{
	  UpdateProcedureWorking = true;
	  TMemoryStream *downstream = new TMemoryStream();
	  AnsiString oldmain_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe";
	  AnsiString update_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe.upd";

	   try
	  {
		  try
		  {
				   IdHTTP1->Get("http://www.xxxx.de/xxxx/Release/xxxx_Update.exe", downstream);

				   if(downstream->Size == DownloadFileSize && downstream->Size > 0 && DownloadFileSize > 0)
				  {

						 downstream->SaveToFile(update_exe);

						 if(FileExists(update_exe) && GetFileSize(update_exe) == DownloadFileSize)
						 {

									 if(MoveFile(oldmain_exe.c_str() , oldmain_exe.c_str() + '.bak'))
									{
										   if(MoveFile(update_exe.c_str() , oldmain_exe.c_str() ))
										   return true;
										   else
										   return false;

									}
									else
									return false;

						 }
						 else
						 return false;

				   }
		  }
		  catch (Exception &ex)
		  {
			// z. B. interne Fehlernachricht
		  }

	  }
	   __finally
	  {

			 UpdateProcedureWorking = false;

			 //Verzeichnis bereinigen oder Datei wiederherstellen falls etwas schief gegangen ist
			 if(FileExists(oldmain_exe) && FileExists(oldmain_exe + ".bak"))
			 DeleteFile(oldmain_exe + ".bak");
			 else
			 {
			   if(FileExists(oldmain_exe + ".bak"))
			   MoveFile(oldmain_exe.c_str() + '.bak', oldmain_exe.c_str() );

			 }

			 if(FileExists(update_exe))
			 DeleteFile(update_exe.c_str());
			 //-------

			 if(IdHTTP1->Connected())
			 IdHTTP1->Disconnect();

			 delete downstream;
	  }

	   return false;

}

Ist das so besser? Ohne ein geschachtelter try Handler ohne Exceptionhandler verursacht Fehlermeldungen als Message auf der GUI, möchte allerdings die möglichen Fehler nur intern behandeln. Daher dieses Konstrukt.

Für diese Funktion ne eigene Klasse zu erstellen um eine RAII Konformität zu erreichen tüftel ich noch aus. Klar ist dies die sauberste Methode.

Ich geh dann mal Hausaufgaben machen bezüglich GetFileSize und ein paar anderen Dingen

[audacia]

Zero01 schrieb:

try __finally ist wirklich eine tolle Sache, nur hat mir das Exception handling gefehlt. Die Embarcadero Docs empfehlen folgende Vorgehensweise [...]

Wie gesagt, try / __finally und try / catch sind zweierlei, das eine ist für die Ressourcenfreigabe bzw. allgemein das Aufräumen nützlich, das andere für die Fehlerbehandlung. Wenn du beides brauchst, dann verschachtelst du eben beides.

Zero01 schrieb:

TMemoryStream *downstream = new TMemoryStream();
      AnsiString oldmain_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe";
      AnsiString update_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe.upd";
      try
      {
            [...]

So macht man das nicht. Wenn du eine Ressource mit try / __finally beschützt, beginnt der try -Block immer direkt nach der Anforderung der Ressource. Also so:

TMemoryStream* downstream = new TMemoryStream;
try
{
    [...]
}
__finally
{
    delete downstream;
}

Außerdem gilt üblicherweise: ein try / __finally -Block pro Ressource. Folgendes ist z.B. fehlerhaft:

TMemoryStream* stream1 = new TMemoryStream;
TMemoryStream* stream2 = new TMemoryStream;
try
{
    [...]
}
__finally
{
    delete stream1;
    delete stream2;
}

Wenn das zweite new fehlschlägt, wird stream1 nie freigegeben. Was du in solchen Fällen machen kannst, ist das hier:

TMemoryStream* stream1 = 0;
TMemoryStream* stream2 = 0;
try
{
    stream1 = new TMemoryStream;
    stream2 = new TMemoryStream;
    [...]
}
__finally
{
    delete stream1;
    delete stream2;
}

Das ist sicher, weil delete auf einen Nullzeiger angewandt wirkungslos ist. Aber noch viel schöner sind natürlich Smartpointer wie std::auto_ptr<> , weil du dann gar kein try / __finally brauchst (weil der Compiler es für dich erzeugt):

std::auto_ptr<TMemoryStream> stream1 (new TMemoryStream);
std::auto_ptr<TMemoryStream> stream2 (new TMemoryStream);
[...]

Zero01 schrieb:

bool __fastcall TForm1::Update_UpdateApplication()
{
	  [...]
	   __finally
	  {
			 UpdateProcedureWorking = false;

			 //Verzeichnis bereinigen oder Datei wiederherstellen falls etwas schief gegangen ist
			 [...]

			 if(IdHTTP1->Connected())
			 IdHTTP1->Disconnect();

			 delete downstream;
	  }
	   return false;
}

Ist das so besser?

Prinzipiell ja, aber es ist noch nicht gut.

Das Problem hier ist, daß du einen __finally -Block für die Freigabe mehrerer Ressourcen verwendest. Das geht unter bestimmten Umständen (s.o.), aber du mußt dich immer fragen, was passiert, wenn eine der Anweisungen im __finally -Block fehlschlägt. Z.B. kann es sein, daß eine der Methoden von TIdHTTP eine Exception wirft. Dann wird alles danach, also insbesondere die Anweisung delete downstream; , nie ausgeführt, und es gibt ein Speicherleck. Deshalb vermeidet man in einem __finally -Block Code, der Exceptions werfen kann.

Um das Speicherleck zu beheben, kannst du entweder Smartpointer und anderen RAII-Guards oder verschachtelte try / __finally -Blöcke verwenden. Manchmal kann es aber auch sinnvoll sein, eine Exception zu unterdrücken: wenn z.B. die Anweisung IdHTTP1->Disconnect (); eine Exception wirft, liegt das wahrscheinlich daran, daß der Server die Verbindung getrennt hat oder sie aus einem anderen Grund nicht mehr besteht: aber das ist dir ja egal, du willst sie sowieso beenden. Dann kannst du in deinem __finally -Block das hier machen:

try
        {
            if (IdHTTP1->Connected ())
                IdHTTP1->Disconnect ();
        }
        catch (Exception& e)
        {
            // ein leerer catch()-Block bedarf immer einer Rechtfertigung! Also:
            // wenn die Verbindung nicht mehr getrennt werden kann, ist sie wohl schon getrennt, dann ist es uns auch egal => Exception wird geschluckt
        }

Du solltest auch beachten, daß Anweisungen wie if (FileExists (...)) DeleteFile (...); eine race condition beinhalten. Was ist, wenn die Datei gerade zwischen den beiden Aufrufen erstellt oder gelöscht wird? Wenn du eine Datei eh löschen willst, kannst du auch einfach unkonditionell DeleteFile() aufrufen und auf den Rückgabewert achten:

if (!DeleteFile (oldmain_exe + ".bak"))
    [...]; // wenn DeleteFile() false zurückgibt, dann gab es die Datei offenbar nicht

(Übrigens gibt es DeleteFile() zweimal: im Win32-API und in der Delphi-RTL, aber sie tun beide dasselbe.)

Beim Aufruf von Win32-Funktionen wie MoveFile() oder GetFileAttributesEx() solltest du außerdem den Rückgabewert beachten. Dir ist sicher das GetLastError() -Pattern bekannt: die meisten Win32-Funktionen geben einen BOOL -Statuswert zurück, der im Fehlerfall FALSE ist, und den genauen Fehler erfragt man dann mit GetLastError() . Die Delphi-RTL hat ein paar praktische Funktionen, um solche Fehler in Exceptions mit einer schön aussagekräftigen Fehlermeldung umzuwandeln:

Win32Check (MoveFile (...)); // wirft eine Exception, wenn MoveFile() fehlschlägt

[Zero01]

Danke Audacia für deine Erläuterungen. Jetzt ist mir einiges bewusst geworden, allerdings bin ich auch etwas irretiert im Bezug auf die boolschen Rückgabewete der Win32 Funktionen. Hast du das so gemeint:

bool __fastcall TForm1::Update_UpdateApplication()
{

		  AnsiString oldmain_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe";
		  AnsiString update_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe.upd";
		  UpdateProcedureWorking = true;

		  TMemoryStream *downstream = new TMemoryStream();
		  try
		  {        try
				  {
				   IdHTTP1->Get("http://www.xxxx.de/xxxx/Release/xxxx_Update.exe", downstream);
				   downstream->SaveToFile(update_exe);
				  }
				  catch(Exception &ex)
				  {
					// ex.ToString()  beinhaltet die Fehlermeldung
					UpdateProcedureWorking = false;
					return false;
				  }

		  }
		   __finally
		  {
			delete downstream;
		  }

		  try
		  {
			 IdHTTP1->Disconnect();
		  }
		  catch (Exception &ex){}  //bleibt unbehandelt

		 try
		 {
			 try
			{
					 //---Body
					 if(downstream->Size == DownloadFileSize && downstream->Size > 0 && DownloadFileSize > 0)
					{

						 if(GetFileSize(update_exe) == DownloadFileSize) //Wenn update_exe nicht vorhanden GetFileSize = 0 und DownloadFilesize muss > 0; Vergleich konsistent
						 {

									 if(MoveFile(oldmain_exe.c_str() , AnsiString(oldmain_exe + ".bak").c_str() ))
									{
										   if(MoveFile(update_exe.c_str() , oldmain_exe.c_str() ))
										   return true;
										   else
										   throw true;

									}
									else
									throw true;

						 }
						 else
						 return false;
					}
					//----*

			  }
			  catch(bool err_state)
			  {

					if(err_state)
					{
					 unsigned int err = GetLastError();
					 String err_message = SysErrorMessage(err);
					 //......tue was mit der Error Message
					 return false;
					}
			  }

		 }
		  __finally
		 {
			UpdateProcedureWorking = false;
		 }

	 /*
		 Wird in eine sepparate Funktion ausgelagert nach der Ausführung der Update Funktion aufgerufen

		 //Verzeichnis bereinigen oder Datei wiederherstellen falls etwas schief gegangen ist
		 try
		 {

			 if(FileExists(oldmain_exe) && FileExists(oldmain_exe + ".bak"))
			 DeleteFile(oldmain_exe + ".bak");
			 else
			 MoveFile(oldmain_exe.c_str() + '.bak', oldmain_exe.c_str() );

			 DeleteFile(update_exe.c_str());
			 //-------
		}
		catch(Exception &ex)
		{
			   unsigned int err = GetLastError();
			   String err_message = SysErrorMessage(err);
		}

	  */

	  return false;

}

[audacia]

Zero01 schrieb:

try
			{
					 //---Body
					 if(downstream->Size == DownloadFileSize && downstream->Size > 0 && DownloadFileSize > 0)
					{

						 if(GetFileSize(update_exe) == DownloadFileSize) //Wenn update_exe nicht vorhanden GetFileSize = 0 und DownloadFilesize muss > 0; Vergleich konsistent
						 {

									 if(MoveFile(oldmain_exe.c_str() , AnsiString(oldmain_exe + ".bak").c_str() ))
									{
										   if(MoveFile(update_exe.c_str() , oldmain_exe.c_str() ))
										   return true;
										   else
										   throw true;

									}
									else
									throw true;

						 }
						 else
						 return false;
					}
					//----*

			  }
			  catch(bool err_state)
			  {

					if(err_state)
					{
					 unsigned int err = GetLastError();
					 String err_message = SysErrorMessage(err);
					 //......tue was mit der Error Message
					 return false;
					}
			  }

Du liebe Güte. Nein, so habe ich das nicht gemeint Das geht viel einfacher.

try
{
    //---Body
    if(downstream->Size == DownloadFileSize && downstream->Size > 0 && DownloadFileSize > 0)
        if(GetFileSize (update_exe) == DownloadFileSize) // wenn update_exe nicht vorhanden, muß GetFileSize = 0 und DownloadFileSize > 0 sein; Vergleich konsistent
        {
            Win32Check (MoveFile (oldmain_exe.c_str (), (oldmain_exe + ".bak").c_str ()));
            Win32Check (MoveFile (update_exe.c_str (), oldmain_exe.c_str ()));
            return true;
        }
}
catch(Exception& e)
{
    // ... mach was mit der Fehlermeldung in e.Message oder e.ToString()
}
return false;

Der Sinn von Exceptions ist, daß dein Programmfluß möglichst nie dadurch unterbrochen wird, daß du explizit überprüfen mußt, ob eine Funktion erfolgreich war. Denn sowas führt zu den if -Kaskaden in deinem Code und ist nicht sehr leserlich. Wenn man Win32Check() verwendet wie in meiner Variante, dann wird eine Exception geworfen, wenn der erste Aufruf fehlschlägt, aber um die kümmerst du dich erst in der catch -Sektion. (Alternativ kann man sie auch nicht behandeln, dann bekommt der Benutzer einfach eine Fehlermeldung angezeigt; aber das weißt du bestimmt.)

Vielleicht hätte ich das vorausschicken sollen, wenn es nicht klar ist: die meisten Funktionen aus dem Win32-API und der C-RTL werfen selbst keine Exceptions. Das liegt daran, daß die Win32-Schnittstelle nur C-Sprachmittel benutzt und es in C eben keine Exceptions gibt. Fehler werden hier über den Rückgabewert kommuniziert (bei Win32-Funktionen mit BOOL-Rückgabewert besagt eben TRUE, daß die Funktion erfolgreich war und FALSE, daß es einen Fehler gab). Die Win32-Funktion, die den Fehler ausgelöst hat, verwendet dann SetLastError() , um eine Fehlernummer zu setzen. Die kannst du dann mit GetLastError() abfragen und mit ein paar anderen Funktionen in eine ordentliche Fehlermeldung umwandeln.

Wenn man aber bei jeder Win32-Funktion den Rückgabewert prüfen muß, führt das schnell wieder zu diesen häßlichen if -Kaskaden und außerdem zu redundantem Code. Deshalb gibt es Win32Check() : du gibst einfach den BOOL-Rückgabewert deiner Win32-Funktion an Win32Check() weiter, und das kümmert sich um all die Details (Fehlernummer herausfinden, in Fehlermeldung umwandeln) und wirft, wenn es einen Fehler gab, eine Exception. D.h. es ist ein praktisches Mittel, um Win32-Funktionen so zu benutzen, als würden sie selbst im Fehlerfall eine Exception werfen, und führt dadurch zu schönerem Code (s.o.).

[Zero01]

Hallo Audacia,
ich weiß gar nicht wie ich dir danken soll. Mit diesem Ergebnis sollten wir auf einen gemeinsamen Nenner kommen:

bool __fastcall TForm1::Update_UpdateApplication()
{

		  AnsiString oldmain_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe";
		  AnsiString update_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe.upd";
          UpdateProcedureWorking = true;

		  //---Download abhandeln
		  TMemoryStream *downstream = new TMemoryStream();
          try
		  {
				   try
                  {
					IdHTTP1->Get("http://www.xxxx.de/xxxx/Release/xxxx_Update.exe", downstream);
				  }
				  catch(Exception &ex)
				  {
					 //-- ex.Message / ex.ToString  beinhaltet die Exception.
					 UpdateProcedureWorking = false;
					 return false;                   //-----Download schlug fehl.
				  }

				  //----

				  try
				  {
					 downstream->SaveToFile(update_exe);
				  }
				  catch(EFCreateError &f_err)
				  {
					 //-- f_err.Message / f_err.ToString  beinhaltet den Fehler.
					 UpdateProcedureWorking = false;
					 return false;                   //-----Speichern schlug fehl.
                  }

		  }
		   __finally
		  {
			 delete downstream;
		  }

		  //Verbindung trennen
		  try
		  {
			 IdHTTP1->Disconnect();
		  }
		  catch (Exception &ex){}  /*bleibt unbehandelt. Sollte wegen einem zuvor aufgerufnen return diese
									 Handler nicht mehr ausgeführt werden wird die Verbindung vom Server
									 Connection Timeout getrennt. */

	   try
	   {
			 try
			 {

				  if(downstream->Size == DownloadFileSize && downstream->Size > 0 && DownloadFileSize > 0)
				 {
					 if(GetFileSize (update_exe) == DownloadFileSize)
					{
						Win32Check (MoveFile (oldmain_exe.c_str (), (oldmain_exe + ".bak").c_str ()));
						Win32Check (MoveFile (update_exe.c_str (), oldmain_exe.c_str ()));
						return true;
					}
				 }

			 }
			 catch(Exception &ex)
			 {
				/*-- Exception MoveFile aus ex.Message / ex.ToString() holen und ggf. in void::Write_Logfile Funktion stopfen.
				 Win32 Check verarbeitet Fehler direkt in diese Exception Klasse.
				*/
			 }

			 return false;  //-- Hier gehts spätestens raus falls etwas fehl schlug.

	   }
	   __finally
	   {
	      UpdateProcedureWorking = false;
	   }

		return false;  //Proforma für den meckernden Interpreter bzw. Debuger, wird allerdings nie aufgerufen.

}
//---------------------------------------------------------------------------
bool __fastcall TForm1::CleanupOrRestoreAfterUpdate()
{

		 AnsiString oldmain_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe";
		 AnsiString update_exe = AnsiString(CurrentDirectory) + "\\xxxx_Update.exe.upd";

		 //--Verzeichnis bereinigen oder Datei wiederherstellen falls etwas schief gegangen ist.
		 try
		 {

				 /*--Lediglich ganz rudimentärer Vergleich. Natürlich sollten hier noch Dateigrößen geprüft oder
				 mit Prüfsummen gearbeitet werden. AUSBAUFÄHIG! */

				 if(Win32Check (FileExists(oldmain_exe)) && Win32Check (FileExists(oldmain_exe + ".bak")))
				 Win32Check (DeleteFile(oldmain_exe + ".bak"));
				 else
				 Win32Check (MoveFile((oldmain_exe + ".bak").c_str() , oldmain_exe.c_str()));

				 //--Diese soll Bedingungslos weg...
				 Win32Check (DeleteFile(update_exe.c_str()));

				 return true;  //--Alles fein, hier gehts raus!

		}
		catch(Exception &ex)
		{
			   /*-- Exception MoveFile aus ex.Message / ex.ToString() holen und ggf. in void::Write_Logfile Funktion stopfen.
				 Win32 Check verarbeitet Fehler direkt in diese Exception Klasse.
				*/
		}

	 return false; //--gar nichts fein, hier gehts raus!

}

[Zero01]

Ahhhh, gaaaanz großer Schnitzer:

if(Win32Check (FileExists(oldmain_exe)) && Win32Check (FileExists(oldmain_exe + ".bak")))

Muss natürlich so sein da ich keine Exceptionbehandlung in dieser Kontrollstruktur benötige und dadurch der else Teil nie ausgeführt wird:

if(FileExists(oldmain_exe) && FileExists(oldmain_exe + ".bak"))

[audacia]

Zero01 schrieb:

Ahhhh, gaaaanz großer Schnitzer: [...]

Genau. Du mußt schon in die Dokumentation schauen, wofür der Rückgabewert gut ist, bevor du Win32Check() verwendest. Z.B. in der Dokumentation zu DeleteFile() steht:

MSDN schrieb:

Return value

If the function succeeds, the return value is nonzero.

If the function fails, the return value is zero (0). To get extended error information, call GetLastError.

Hier ist also Win32Check() richtig. Aber in der Doku zu FileExists() aus der Delphi-RTL steht nichts von GetLastError() oder sonst einem Fehlercode, also brauchst du kein Win32Check() .

Allgemein braucht man das nur bei Win32-Funktionen (auch da nicht immer!); die Delphi-RTL und die VCL werfen bereits Exceptions, wenn etwas schiefgeht.

[Zero01]

Auch nicht schlecht:

}
           __finally
          {
             delete downstream;
          }

 [].....

       try
       {
             try
             {

                  if(downstream->Size == DownloadFileSize

Ich habe noch ein paar meiner "Blindheitsfehler" (s.o.) ausgemerzt und den Code heute über Stunden in allen erdenklichen Situationen getestet. Selbst wenn der Download des Hauptprogramms mittendrin abbricht durch z.B. gekappte Internetleitung wird die Installation nicht beschädigt und das Programm startet automatisiert in der alten Version und versucht das Update zu einem späteren Zeitpunkt erneut. Bisher läuft das ganze schon sehr robust.

Allerdings hatte mich die Indy Komponente ein wenig auf Trapp gehalten. Wer sowas nachmachen möchte sollte bitte bedenken das Indy Komponenten dieser Art im blocking Mode arbeiten und für Threads ausgelegt sind. Aber auch der schönste Thread bringt nichts wenn man nicht die Parameter "ConnectionTimeout" und "ReadTimeout" beachtet. Ohne diese Parameter bleibt der Programmablauf bis zum Sankt Nimmerleinstag im Aufruf "GET" stehen falls die Verbindung just in dem Moment abbricht.

Audacia, allen Dank gilt dir. Habe sehr viel Quelltext mit deinen Tips optimieren können und meine Denkweise in einigen Dingen geändert.