Was haltet ihr von diesem Code?

[Olli73]

Delphi-Quellcode:

const
  ANY_SIZE = 1;
  
type

  MIB_IPADDRROW = record
    dwAddr: LongInt;
    dwIndex: LongInt;
    dwMask: LongInt;
    dwBCastAddr: LongInt;
    dwReasmSize: LongInt;
    unused1: word;
    unused2: word;
  end;

  MIB_IPADDRTABLE = record
    dwNumEntries: LongInt;
    table: array [1 .. ANY_SIZE] of MIB_IPADDRROW;
  end;

  TMIBIpAddrTable = MIB_IPADDRTABLE;
  PMIBIpAddrTable = ^TMIBIpAddrTable;


function GetNetworkAdapters(out AAdapters: TNetworkAdapterInfos): Boolean;
var
  pIfTable: PMIBIfTable;
  pIpTable: PMIBIpAddrTable;
  ifTableSize, ipTableSize: LongInt;
  CurrentAdapter: TNetworkAdapterInfo;
  MacAddress: string;
  i, j, k: Integer;
  sAddr, sMask: in_addr;
  IPAddresses, IPMasks: TDictionary<Integer, AnsiString>;
  IPString: AnsiString;
begin
  IPAddresses := TDictionary<Integer, AnsiString>.Create;
  try
    IPMasks := TDictionary<Integer, AnsiString>.Create;
    try
      ipTableSize := SizeOf(TMIBIpAddrTable);
      GetMem(pIpTable, ipTableSize);
      if GetIpAddrTable(pIpTable, ipTableSize, False) = ERROR_INSUFFICIENT_BUFFER then
      begin
        FreeMem(pIpTable, SizeOf(TMIBIpAddrTable));
        GetMem(pIpTable, ipTableSize);
      end;
      try
        if GetIpAddrTable(pIpTable, ipTableSize, False) = NO_ERROR then
        begin
          for k := 1 to pIpTable^.dwNumEntries do
          begin
            sAddr.S_addr := pIpTable^.table[k].dwAddr;
            sMask.S_addr := pIpTable^.table[k].dwMask;
            if not IPAddresses.ContainsKey(pIpTable^.table[k].dwIndex) then
              IPAddresses.Add(pIpTable^.table[k].dwIndex, inet_ntoa(sAddr));
            if not IPMasks.ContainsKey(pIpTable^.table[k].dwIndex) then
              IPMasks.Add(pIpTable^.table[k].dwIndex, inet_ntoa(sMask));
          end;
        end;
      finally
        if Assigned(pIpTable) then
          FreeMem(pIpTable, ipTableSize);
      end;

...

end;

Hier wird ein Array [1..1] per Pointer und GetMem dazu missbraucht, mehr Daten aufzunehmen, als eigentlich in das Array passen. Da hat bei mir dann der RangeCheck zugeschlagen und ich musste diesen an der Stelle abschalten.

Aber macht man sowas wirklich?

[freimatz]

Informatiker-Antwort: man schon, ein guter Entwickler nicht.

Und wenn wir schon dabei sind:
1. TDictionary<Integer, AnsiString>
a) 3x, dann besser Typ definieren. Am Besten mit einem Namen so dass man versteht was das soll.
b) AnsiString: macht man das heuzutage noch?
2. Methode zu lang
3. "Was hatet ihr von diesem Code?" Also haten muss man den Code nicht, es gibt wesentlich schlimmeres

[Der schöne Günther]

Ja, ich finde das auch schon zu verschachtelt und unübersichtlich, aber ich glaube das war gar nicht gemeint, sondern die Geschichte mit den ANY_SIZE-Array:
https://devblogs.microsoft.com/oldne...26-00/?p=38043

Zitat:

Also haten muss man den Code nicht, es gibt wesentlich schlimmeres

Doch, ich hate ihn jetzt schon.

Spätere/Neuere Methoden aus der WinApi haben es dann besser gemacht und z.B. eine LinkedList als Datenstruktur zurückgegeben:

https://learn.microsoft.com/en-us/wi...ptersaddresses

https://learn.microsoft.com/en-us/wi...r_addresses_lh

[Olli73]

Zitat von freimatz:

2. Methode zu lang

Also bei Methoden, die weniger als ein paar Tausend Zeilen haben, rege ich mich erst gar nicht mehr auf ...

Ist halt teilweise uralter Code von Anderen, ich muss halt damit zurechtkommen. Das Problem hier war, dass der RangeCheck (im Release war der bei dem Projekt deaktiviert) zugeschlagen hat und ich der Meinung bin, Pointer und GetMem sollte man in solchen Fällen vermeiden, sonst kann man gleich C++ machen.

[himitsu]

Seit ein paar Delphi-Versionen ist die Bereichsprüfung standardmäßig aktiv.
[0..0] oder [1..1] gibt es öfters, aber bei größerem Index knallen diese Prüfungen.

Tipp: [1 .. MaxInt div SizeOf(MIB_IPADDRROW)] of MIB_IPADDRROW;
Alternativ die Pointer-Arithmetic aktivieren
und anstatt PMIBIpAddrTable = ^TMIBIpAddrTable;
ein PMIBIpAddr = ^MIB_IPADDRROW; , welches sich auch via .table[k] zugreifen lässt.

Achtung, da beginnt es mit Index 0, also so als wäre es [0..x]

Und wenn 0, dann beim for k := 0 to pIpTable^.dwNumEntries - 1 do aufpassen, da dwNumEntries ein DWORD ist und es somit Probleme gibt, wenn dwNumEntries 0 wäre, da bei -1 dann 4 Millarden, anstatt -1 rauskommt.
OK, zum Glück ist heutzutage standfardmäßig die Überlaufprüfung aktiv und würde bei 0 - 1 schön knallen.

Also besser to Integer(pIpTable^.dwNumEntries) - 1 do (größer als 2 Milliarden wird dwNumEntries hoffentlich nie sein)
oder vorher ein if pIpTable^.dwNumEntries > 0 then for ... .

[Blup]

Speicher muss man erst für den zweiten Aufruf der Funktion reservieren:

Delphi-Quellcode:

...
      ipTableSize := 0;
      pIpTable := nil;
      if GetIpAddrTable(pIpTable, ipTableSize, False) = ERROR_INSUFFICIENT_BUFFER then
      begin
        GetMem(pIpTable, ipTableSize);
        try
          if GetIpAddrTable(pIpTable, ipTableSize, False) = NO_ERROR then
          begin
...
          end;
        finally
          FreeMem(pIpTable);
        end;
     end;
...

[Olli73]

Da das nur ein Pointer ist, der noch nicht auf einen Record zeigt, muss man doch Speicher reservieren!?

[himitsu]

Es kommt drauf an.

Schau in die Hilfe.
GetIpAddrTable

Es gibt API,
* die wollen vom Nutzer Speicher haben
* Vielen wollen Speicher bekommen, aber ihnen reicht z.B. eine Record-Variable
* * hier würde sie auch reichen, wenn es nur eine IP gibt (oder wenn man sich selbst den Typen anpasst und z.B. [0..9] für maximal 10 IPs)
* Andere geben einen Zeiger auf internen Speicher raus
* oder sie erzeugen selbst den Speicher, welchen man dann ebenfalls freigeben muß

Zitat:

Delphi-Quellcode:

type
  MIB_IPADDRROW = ...

Warum?

Du verwendest eine API, welche nicht nur einen billigen Pointer als Typ hat, und da ist es natürlich einfacher/besser, auch diesen Typen dann zu verwenden.
Sonst kann es auch schnell passieren, dass der Compiler abraucht, wenn es ihm nicht gefällt, dass es "unterschiedliche" Typen sind.

Winapi.IpHlpApi
Winapi.IpRtrMib

PS: Windows.NetworkManagement.IpHelper aus'm GetIt WinMD
https://www.delphipraxis.net/214473-...vor-winmd.html

[Olli73]

Ja, aber in dem Fall will die API doch reservierten Speicher, und ein Pointer irgendwohin, schreibt halt auch "irgendwohin" oder crasht.

Außerdem ist der Code ja nicht von mir. Mir gefällt ja auch nicht, dass ich die Bereichsüberprüfung lokal abschalten musste.

[Blup]

Die Funktion prüft erst ob ipTableSize genügend groß ist um die Ergebnisse aufzunehmen.
Bei 0 ist das nicht der Fall.
Deshalb gibt die Funktion in ipTableSize die tatsächlich benötigte Größe zurück und setzt Result auf ERROR_INSUFFICIENT_BUFFER.
Auf den Zeiger greift die Funktion in diesem Fall garnicht zu.