Покритикуйте патч к systemd-cryptsetup

+7

Что такое 7 ?

Это их странный стиль работы с строками. Я его придерживался. Поскольку мы разбираем строку вида «header=blablabla», то +7 даст нам подстроку blablabla, пропустив первые 7 символов («header=»), банальная арифметика с указателями.

Вот оригинал: http://cgit.freedesktop.org/systemd/systemd/tree/src/cryptsetup/cryptsetup.c

Кто не в теме не поймёт. Магическое число. Заверни в

/*эта хрень тут для того, ну вы поняли*/
const int придумай_имя = 7;

или #define ИМЯ 7 

Нет, если делать понятнее, то уж strlen("header="), потому что всё равно в компайл-тайме посчитается. Но там такой стиль. Достаточно странный, но такой.

Ну хоть так, главное от магической 7 избавится.

int ignore = strlen("header=");

Нарушение стиля оформления кода (в данном конкретном случае — отбивка пробелами скобок и операторов) выдает небрежность автора, следовательно, такой патч можно вообще не рассматривать.

отбивка пробелами скобок

Мой косяк.

и операторов

В оригинальном файле есть оба варианта - с отбивкой пробелами и без отбивки.

C этим всё как раз хорошо, кроме пробела между скобкой и arg_header вот здесь:

( arg_header ? argv[3] : NULL)

UPD: а, если речь про отбивку от фигурной скобки — то да.

return -1; // fatal

-EINVAL

Header path '%s' is not absolute.

Duplicate header= options

На правах придирки: "...is not absolute, refusing." и "...options, refusing.".

Это что-то вроде «общего шаблона сообщений об ошибках» в местном коде — когда в конце сообщения указана реакция программы на ошибку.

нет чтобы регэкспами парсить

o_O смысл?

так Ъ :)

Что будет, если юзверь после header= сделает пробел? Оно выдаст осмысленную ошибку?

Отлично, хорошие замечания по теме, и про return, и про шаблон.

В /etc/crypttab:

The fourth field, if present, is a comma-delimited list of options.

Похоже, что параметр header= будет пуст, так как по логике список закончился. Но я не проверял.

это ж блин эталонный быдлокод

неужели трудно было какой-нибудь аналог getopt() / getopt_long() придумать для разбора параметров?

Оно скажет, что «Header path ' <путь>' is not absolute». Достаточно осмысленно, как по мне.

Кто не в теме не поймёт. Магическое число. Заверни в

Так как раз делать нельзя, магическое имя с неизвестным значением ещё больше непонятнее чем просто число. Тем более по 7ке легко догадаться что это. Если хочется что-то дефайнить, то сам литерал «header=» и брать от него sizeof().

Оно скажет, что «Header path ' <путь>' is not absolute». Достаточно осмысленно, как по мне.

Ну да. Юзверь будет долго втыкать в

Header path ' /etc/somefile' is not absolute

и гадать, почему чёрное это белое. Только опытные наступатели на грабли заметят пробел

Заметить пробел между кавычкой и слешом в моноширинном шрифте — просто.

Ок. Пусть идёт в продакшен в таком виде, я только за. Пусть юзеры видят, с чем и с кем имеют дело :)

Вот этот фрагмент --

-        if (!arg_type || streq(arg_type, CRYPT_LUKS1))
+        if (!arg_type || streq(arg_type, CRYPT_LUKS1)) {
                 r = crypt_load(cd, CRYPT_LUKS1, NULL);
+                if (r < 0) {
+                        log_error("crypt_load() failed on device %s.\n", crypt_get_device_name(cd));
+                        return r;
+                }
+
+                if (data_device)
+                        r = crypt_set_data_device(cd, data_device);
+        }

на мой взгляд, неверен. arg_type == NULL имеет семантику «автоподбора типа», т. е. ошибка должна игнорироваться. Можно сделать так:

        if (!arg_type || streq(arg_type, CRYPT_LUKS1)) {
                r = crypt_load(cd, CRYPT_LUKS1, NULL);
                if (r == 0) {
                        <твой код>
                }
        }

        if (!arg_type || streq(arg_type, CRYPT_LUKS1)) {
                r = crypt_load(cd, CRYPT_LUKS1, NULL);
                if (arg_type && r < 0)
                        return log_error_errno(r, "Loading of cryptographic parameters failed: %m");

                <твой код>
        }

Ну да. Юзверь будет долго втыкать в

Header path ' /etc/somefile' is not absolute

и гадать, почему чёрное это белое. Только опытные наступатели на грабли заметят пробел

В Linux имя файла ВНЕЗАПНО может начинаться с пробела, поэтому ' /etc/somefile' может быть валидным относительным путем.

А, не, последний вариант некорректен. Очень кривой code flow здесь... Вынести бы обработку LUKS1 и PLAIN в отдельные функции.

Действительно. Но всё равно пробел лучше экранировать или в кавычки всю строку заворачивать

Это вопрос к тому, кто подаёт программе такой ввод.

В случае crypttab такого произойти не может, т. к. там столбцы разделяются пробельными символами. А если программа запускается вручную — ну, ССЗБ.

Это быдлокод и magic numbers. Нельзя что-ли getopt прикрутить уже? Если нельзя, надо написать автомат и простенькую грамматику. В конце концов:

const char optName[] = "header="; // в идеале все опции куда-то вынести, чтобы было видно, что за опции вообще есть.
const optLen = strlen(optName);

потому что всё равно в компайл-тайме посчитается

Экономия на спичках, в таком месте даже об этом задумываться не стоит.

Пробелы не считаются.

"someprog -a abc -b def" и "someprog    -a      abc     -b      def"

Ты не так прочёл код. Это всё происходит внутри одного параметра.

это смотря как аргументы парсятся

Читал патч :) Точнее даже комментарий к патчу.

Ну если там будет

progname "--header=    my path     "

Соответствуй стилю кода вокруг:

if/* вот тут пробел нужен */(!path_is_absolute(option+7)) {
/* и чуть ниже тоже самое */

return -1; // fatal

-std=c99?

Но там такой стиль. Достаточно странный, но такой.

То есть, если им прислать нормальный код, без magic number-ов — его отклонят?

Это-ж systemd! Они и CRT скоро «перепишут», такими темпами.

в компайл-тайме посчитается

Разве? А я думал, только sizeof считается на этапе компиляции.

засунуть header — это хорошо

а offset можно как нибудь засунуть? в другом патче конечно же .. (или уже кто-то засунул?)

Я откуда знаю. Не ревьюер.

Если просто прислать патч, в котором в одном месте сделано как-то по-другому — скорее всего, отклонят. И правильно сделают.

А если пройтись чем-нибудь по всему дереву исходников и заменить все вхождения — наверняка примут.

А он тебе что написал? Compile time.

Ыыы, патч таки приняли в апстрим с небольшими правками:

http://cgit.freedesktop.org/systemd/systemd/commit/?id=7376e83528