GetLastError und falsche Fehlermeldungen, z.b. in TFileStream

[mael]

Hallo,

Nach einer kurzen Diskussion in der Shoutbox wollte ich mal ein konkretes Beispiel geben. Man kann sich da nur zu kurz und daher unklar ausdrücken, und wahrscheinlich meinten wir dasselbe.

Der folgende Code gibt den Fehler 3 aus.

Delphi-Quellcode:

procedure TForm1.Button1Click(Sender: TObject);
var
  Filename: string;
begin
  Filename := '';
  if CreateFile(PChar(Filename), GENERIC_READ, 0, nil, OPEN_EXISTING, 0, 0) = INVALID_HANDLE_VALUE then
    ShowMessage(IntToStr(GetLastError));
end;

Dieser aber 123 weil vorher noch ExpandFileName aufgerufen wird.

Delphi-Quellcode:

procedure TForm1.Button2Click(Sender: TObject);
var
  Filename: string;
begin
  Filename := '';
  if CreateFile(PChar(Filename), GENERIC_READ, 0, nil, OPEN_EXISTING, 0, 0) = INVALID_HANDLE_VALUE then
  begin
    ExpandFileName(Filename); // ruft WinAPI funktionen auf
    ShowMessage(IntToStr(GetLastError));
  end;
end;

So ein ähnliches Problem kriegt man auch mit TFileStream.Create wo das nicht so direkt zu erkennen ist:

Delphi-Quellcode:

    inherited Create(FileCreate(AFileName, LShareMode, Rights));
    if FHandle = INVALID_HANDLE_VALUE then
      raise EFCreateError.CreateResFmt(@SFCreateErrorEx, [ExpandFileName(AFileName), SysErrorMessage(GetLastError)]);

So eine Art Code ist leider in vielen Teilen der VCL zu finden, zumindest in XE3. Das heißt es könnten manchmal falsche Fehlermeldungen kommen. Hier ist es nicht so auffällig weil:

SysErrorMessage(123) = 'Die Syntax für den Dateinamen, Verzeichnisnamen oder die Datenträgerbezeichnung ist falsch' und
SysErrorMessage(3) = 'Das System kann den angegebenen Pfad nicht finden'

Ruft man TFileStream.Create folgendermaßen auf bekommt man auch ganz folgerichtig die Fehlermeldung für SysErrorMessage(123) und nicht SysErrorMessage(3):
TFileStream.Create('', fmOpenRead or fmShareDenyNone);

Das ist in dem Fall nicht so schlimm, aber prinzipiell ist es unsauber und hat Potenzial für unerklärliche Probleme.

Es ist also besser den Fehlercode nach einem WinAPI-Aufruf zu speichern wenn man denn diesen Fehlercode ausgeben möchte (falls diese WinAPI-Funktion False zurückgegeben hat oder anders einen Fehler signalisiert hat). Ruft man aber dazwischen noch eine andere WinAPI-Funktion auf, auch wenn das implizit geschieht durch andere Funktionen, gibt es das Potenzial dafür dass GetLastError den "falschen" Fehlercode zurückgibt.

Daher ist es gut dass wenn man den Fehlercode benötigt, ihn direkt nach dem API-Aufruf speichert, ohne sonstige Funktionen dazwischen aufzurufen, und dann an der gewünschten Stelle ausgibt, z.B. mit Hilfe von SysErrorMessage().

[Der schöne Günther]

Wir meinten beide tatsächlich wohl das gleiche. Ich sehe nur nicht wozu der direkte "GetLastError()"-Aufruf notwendig ist, es gibt ja

Win32Check( CreateFile(..) <> INVALID_HANDLE_VALUE ); oder
if (CreateFile(..) = INVALID_HANDLE_VALUE) then RaiseLastOSError(); Die Fundstelle in TFileStream.Create() ist wirklich gut. Wenn man im Internet nach "tfilestream create getlasterror" sucht findet man auch einen QC-Eintrag bei Embarcadero aber der Server streikt ausnahmsweise mal wieder.


Was ich in der Shoutbox noch meinte war der umgekehrte Fall: Manche WinApi-Routinen rufen SetLastError(ERROR_SUCCESS) nicht auf wenn sie erfolgreich waren. Deshalb sollte man nicht GetLastError() nehmen und einen Fehler annehmen wenn dieser nicht ERROR_SUCCESS (also Null) ist.

[mael]

Zitat von Der schöne Günther:

Wir meinten beide tatsächlich wohl das gleiche. Ich sehe nur nicht wozu der direkte "GetLastError()"-Aufruf notwendig ist, es gibt ja

Win32Check( CreateFile(..) <> INVALID_HANDLE_VALUE ); oder
if (CreateFile(..) = INVALID_HANDLE_VALUE) then RaiseLastOSError();

Z.b. wenn man Fehlermeldungen zusammensetzen möchte: <Meine Fehlermeldung><OS-Fehlermeldung>.
Oder man will einfach auf konkrete Fehlercodes reagieren um z.B. eine Datei die gesperrt war wieder nach x Sekunden zu laden zu versuchen, aber wenn der Dateiname sowieso ungültig ist es lassen.

Zitat von Der schöne Günther:

Manche WinApi-Routinen rufen SetLastError(ERROR_SUCCESS) nicht auf wenn sie erfolgreich waren. Deshalb sollte man nicht GetLastError() nehmen und einen Fehler annehmen wenn dieser nicht ERROR_SUCCESS (also Null) ist.

Ist auch ein guter Hinweis. Vielleicht sollte man das irgendwo in der CodeLib oder so aufnehmen?

[himitsu]

Zitat:

Manche WinApi-Routinen rufen SetLastError(ERROR_SUCCESS) nicht auf wenn sie erfolgreich waren.

Was im Prinzip ja richtig ist, da diese API GetLastError und nicht GetLastStatus heißt.

Aber, wie in der Shoutbox bereits gesagt wurde, sollte/muß man den gewünschten Fehlercode direkt nach der "fehlerhaften" API abrufen und zwischenspeichern, wenn er nicht sofort verwendet wird, damit das Ergebnis nicht eventuell durch eine andere "fehlerhafte" API zwischenzeitlich verändert werden könnte.

Delphi-Quellcode:

constructor EFileStreamError.Create(ResStringRec: PResStringRec; const FileName: string);
begin
  inherited CreateResFmt(ResStringRec, [ExpandFileName(FileName), SysErrorMessage(GetLastError)]);
end;

Hier könnte nicht nur ExpandFileName quer schießen, sondern auch der Constructor, da doch im Begin der Speicher für die Instanz reserviert wird.

[BerndS]

GetLastError wird leider auch dann auf 0 gesetzt, wenn der Code in einer DLL ausgelagert ist und man diesen im Except abfragt.

Wünschenswert wäre es, wenn EFileStreamError um LastError erweitert wird.

Bis dahin verwende ich (wenn möglich) meine eigenen TFSFileStream.

Delphi-Quellcode:

TFSFileStream = class(THandleStream)
  strict private
    FFileName: string;
  public
    constructor Create(const AFileName: string; Mode: Word); overload;
    destructor Destroy; override;
    property FileName: string read FFileName;
  end;

  EFSFileStreamError = class(EFileStreamError)
  private
    FLastError: Cardinal;
  public
    constructor Create(ResStringRec: PResStringRec; const FileName: string);
    property LastError: Cardinal read FLastError;
  end;
  EFSCreateError = class(EFSFileStreamError);
  EFSOpenError = class(EFSFileStreamError);