Чиним hanstunnel на openwrt (вычисление контрольной суммы сетевого пакета)

[troydm]

я думаю было бы неплохо если вы бы засабмитили этот фикс

[zabbius]

знать бы еще как…

[ToSHiC]

github.com/friedrich/hans
Форкаете, патчите код (обязательно закрыв изменения ифдефами), коммитите, делаете пулл реквест. Всё!

[jcmvbkbc]

Во-первых огребёте с выравниванием, т.к. никто не сказал что пришедший указатель 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;
}

[zabbius]

Согласен, если ntohs не макрос, то так будет лучше.

[jcmvbkbc]

Традиционно один патч — одна проблема. Этот патч фиксит неправильно считаемую контрольную сумму.
Вы предлагаете патч для оптимизации. Обычно вместе с этим требуется какое-нибудь обоснование: цифры, показывающие улучшение.

[jcmvbkbc]

проблема одна — ф-ция не нейтральна к endianess + возможен доступ к невыравненным

Плюс между слагаемыми просто оттеняет то, что проблема одна…

[zabbius]

Т.е. если автор hanstunnel поговнокодил — можно говнокодить? Я хотя бы assert-ов добавил бы, на то что указатель выравненный.

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

[zabbius]

Оригинал не смотрел

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

Смотрите код целиком. Не будет gcc выделять по new блок с плохим выравниванием. Код и так endianess-нейтральный. Сетевой контрольной сумме пофигу на endian, а последний байт ставится на место через ntohs.

[zabbius]

Изменения на портабельность не повлияли. Допущения обеспечивает класс, которому принадлежит метод. Кстати, если код не портабелен, то как он работает на Intel, ARM и MIPS?

[zabbius]

Я просто пытаюсь донести, что этот код в том контексте, в котором он используется в приложении, не нуждается в дополнительных assert'ах и проверках. Без контекста и к i++ придраться можно, ибо, например, не thread-safe.
Кстати, если напишете тесты, которые покрашат hanstunnel на MIPS и ARM, будет интересно.

[zabbius]

Кстати, можно тогда посмотреть на пример вашего решения данной проблемы? То есть как бы на моем месте проблему решали вы? Имеется в виду в данном конкретном случае.

[zabbius]

Основное отличие тут — побайтовое чтение. Сам по себе порядок байтов абсолютно не важен в силу алгоритма подсчета.

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

    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 в конструкторе. То есть смещение всегда четное.

Предлагаю сойтись на том, что проверки и побайтовое чтение нужны в общем случае, особенно, если код отдается как библиотека.

[jcmvbkbc]

Теоретическая часть по ссылке из поста отлично объясняет, почему суммирование с циклическим переносом работает одинаково и на be и на le. Рекомендую ознакомиться.

TL;DR: смена порядка байт при чтении компенсируется сменой порядка байт при записи, а перенос всё равно циклический.

[Ghool]

А можно готовый вариант в виде устанавливаемого пакета для чайников?