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

[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!?

[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