Pull to refresh

Comments 25

я думаю было бы неплохо если вы бы засабмитили этот фикс
github.com/friedrich/hans
Форкаете, патчите код (обязательно закрыв изменения ифдефами), коммитите, делаете пулл реквест. Всё!
UFO just landed and posted this here
Во-первых огребёте с выравниванием, т.к. никто не сказал что пришедший указатель 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;
}
Согласен, если ntohs не макрос, то так будет лучше.
UFO just landed and posted this here
Традиционно один патч — одна проблема. Этот патч фиксит неправильно считаемую контрольную сумму.
Вы предлагаете патч для оптимизации. Обычно вместе с этим требуется какое-нибудь обоснование: цифры, показывающие улучшение.
UFO just landed and posted this here
проблема одна — ф-ция не нейтральна к endianess + возможен доступ к невыравненным

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

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

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

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

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

    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 just landed and posted this here
Теоретическая часть по ссылке из поста отлично объясняет, почему суммирование с циклическим переносом работает одинаково и на be и на le. Рекомендую ознакомиться.

TL;DR: смена порядка байт при чтении компенсируется сменой порядка байт при записи, а перенос всё равно циклический.
UFO just landed and posted this here
А можно готовый вариант в виде устанавливаемого пакета для чайников?
Sign up to leave a comment.

Articles