C++
Comments 25
+1
я думаю было бы неплохо если вы бы засабмитили этот фикс
UFO landed and left these words here
0
Во-первых огребёте с выравниванием, т.к. никто не сказал что пришедший указатель data — выравнен по 2 байта

Оригинальный код на эту тему тоже не парится, так что норм.
Во-вторых, нужно сделать код endianess-нейтральным, а не так как сейчас

Я бы сделал так:
uint16_t Echo::icmpChecksum(const char *data, int length)
{
    uint16_t *data16 = (uint16_t *)data;
    uint32_t sum = 0;
    
    for (sum = 0; length > 1; length -= 2)
        sum += *data16++;
    if (length == 1) {
        char tail[2] = {*(char *)data16, 0};
        sum += *(uint16_t *)tail;
    }
    sum = (sum >> 16) + (sum & 0xffff);
    sum += (sum >> 16);
    return ~sum;
}
UFO landed and left these words here
0
Традиционно один патч — одна проблема. Этот патч фиксит неправильно считаемую контрольную сумму.
Вы предлагаете патч для оптимизации. Обычно вместе с этим требуется какое-нибудь обоснование: цифры, показывающие улучшение.
UFO landed and left these words here
0
проблема одна — ф-ция не нейтральна к endianess + возможен доступ к невыравненным

Плюс между слагаемыми просто оттеняет то, что проблема одна…
UFO landed and left these words here
0
Т.е. если автор hanstunnel поговнокодил — можно говнокодить? Я хотя бы assert-ов добавил бы, на то что указатель выравненный.

Тогда уж надо вообще всю прогу рефакторить. Только вот с таким подходом все время уйдет на один сплошной рефакторинг.
0
Оригинал не смотрел

Не читал, но осуждаю (с)

Смотрите код целиком. Не будет gcc выделять по new блок с плохим выравниванием. Код и так endianess-нейтральный. Сетевой контрольной сумме пофигу на endian, а последний байт ставится на место через ntohs.
UFO landed and left these words here
0
Изменения на портабельность не повлияли. Допущения обеспечивает класс, которому принадлежит метод. Кстати, если код не портабелен, то как он работает на Intel, ARM и MIPS?
UFO landed and left these words here
0
Я просто пытаюсь донести, что этот код в том контексте, в котором он используется в приложении, не нуждается в дополнительных assert'ах и проверках. Без контекста и к i++ придраться можно, ибо, например, не thread-safe.
Кстати, если напишете тесты, которые покрашат hanstunnel на MIPS и ARM, будет интересно.
UFO landed and left these words here
0
Кстати, можно тогда посмотреть на пример вашего решения данной проблемы? То есть как бы на моем месте проблему решали вы? Имеется в виду в данном конкретном случае.
UFO landed and left these words here
0
Основное отличие тут — побайтовое чтение. Сам по себе порядок байтов абсолютно не важен в силу алгоритма подсчета.

Теперь смотрим единственное использование метода.

    EchoHeader *header = (EchoHeader *)(sendBuffer + sizeof(IpHeader));
    header->type = reply ? 0: 8;
    header->code = 0;
    header->id = htons(id);
    header->seq = htons(seq);
    header->chksum = 0;
    header->chksum = icmpChecksum(sendBuffer + sizeof(IpHeader), payloadLength + sizeof(EchoHeader));


где IpHeader имеет размер 20 байт, а sendBuffer выделен по new в конструкторе. То есть смещение всегда четное.

Предлагаю сойтись на том, что проверки и побайтовое чтение нужны в общем случае, особенно, если код отдается как библиотека.
UFO landed and left these words here
+1
Теоретическая часть по ссылке из поста отлично объясняет, почему суммирование с циклическим переносом работает одинаково и на be и на le. Рекомендую ознакомиться.

TL;DR: смена порядка байт при чтении компенсируется сменой порядка байт при записи, а перенос всё равно циклический.
UFO landed and left these words here
0
А можно готовый вариант в виде устанавливаемого пакета для чайников?
Only those users with full accounts are able to leave comments.  , please.