Re: [make-initrd] [PATCH 1/3] Reimplement ueventd

[Alexey Gladkov <legion@altlinux.ru>]

On Thu, May 18, 2023 at 09:39:53AM +0300, Leonid Krivoshein wrote:
> Ой, второй раз уже кнопкой промахиваюсь. ((
> 
> 
> On 5/18/23 08:16, Leonid Krivoshein wrote:
> > Привет!
> >
> >
> > On 5/15/23 13:43, Alexey Gladkov wrote:
> >> [...]
> >>>> +    pid_t pid = vfork();
> >>>> +
> >>>> +    if (pid < 0) {
> >>>> +        err("fork: %m");
> >>>> +
> >>>> +    } else if (!pid) {
> >>>> +        execve(argv[0], argv, environ);
> >>> Компилятор не ругается здесь только потому, что подключен <unistd.h>
> >>> через "ueventd.h", а ругаться у него целых два повода (execve и
> >>> environ).
> >> Если ты предполагаешь, что тут должна быть ругань про отсутствие
> >> определения execve и environ, то ты ошибаешься. Ругани тут нет так как
> >> определения приезжают через ueventd.h.
> >>
> >> Или ты про какие поводы для ругани ?
> >
> > Так я и написал, что ругани нет только потому, что подключен <unistd.h>.
> >
> >
> >>> Это я к тому, что в твоём заголовочном файле он подключен для
> >>> logging.c, а не для queue-processor.c. Согласно man 7 environ можно не
> >>> объявлять эту переменную в коде, если <unistd.h> подключается с
> >>> _GNU_SOURCE, т.е. как раз твой случай.
> >> Что значит подключён для logging.c ? unistd.h просто подключён в
> >> ueventd.h.
> >
> > А это было сказано вот к чему:
> >
> >> > Обычно они все выносятся наверх, потому и заголовочные. > Такой 
> >> подход убережёт от понимания ошибок компилятора при появлении в > 
> >> дальнейшем конфликтов в именах между твоим и библиотечным кодом.
> >
> >> Я так делаю чисто для удобства чтобы понимать, что include не нужен
> >> определениям выше. Когда портянка include собрана сверху, то не всегда
> >> ясно для чего include добавлен. Но для меня это совершенно не критично.
> >
> >
> > Получается, что <dirent.h> упоминается дважды, а <unistd.h> только 
> > один раз и не для queue-processor.c. Как бы там ни было, это необычно 
> > читать. Обычно рядом с заголовочным файлом пишут, для чего его 
> > подключают. И если речь о подключении в твоём заголовочном файле, то 
> > вот эти все стандартные нужны только при совместном использовании 
> > типов, иначе их лучше подключать не в заголовочном файле.
> >
> >
> >> Весь этот код компилируется с глобальным _GNU_SOURCE.
> >>
> >>>> +        fatal("exec: %s: %m", argv[0]);
> >>>> +    } else {
> >>>> +        int status = 0;
> >>>> +
> >>>> +        if (waitpid_retry(pid, &status, 0) != pid || 
> >>>> !WIFEXITED(status))
> >>>> +            info("%s: session=%lu: %s failed",
> >>>> +                 queue->q_name, session, handler_file);
> >>> Насчёт "session=%lu..." только лишь глядя в этот код понял, что и сам в
> >>> не публичном коде опрометчиво использую uint64_t с таким
> >>> форматированием, хотя везде по коду должно быть "session=" PRIu64 
> >>> "...".
> >>> Нужно подключить заголовочный файл <inttypes.h> либо использовать
> >>> какое-то приведение типов. Пренебречь этим, как сейчас, можно только
> >>> полагаясь на конкретные платформы.
> >> Я знаю про PRIu64, но его использование не понимает clang и 
> >> получается вот
> >> так:
> >>
> >> datasrc/ueventd/queue-processor.c:40:27: warning: format specifies 
> >> type 'char *' but the argument has type 'uint64_t' (aka 'unsigned 
> >> long') [-Wformat]
> >>                                  queue->q_name, session, handler_file);
> >>                                                 ^~~~~~~
> >> datasrc/ueventd/ueventd.h:70:69: note: expanded from macro 'rd_info'
> >> #define rd_info(format, arg...) rd_log_message(LOG_INFO, format, ##arg)
> >> ~~~~~~    ^~~
> >> datasrc/ueventd/queue-processor.c:40:36: warning: data argument not 
> >> used by format string [-Wformat-extra-args]
> >>                                  queue->q_name, session, handler_file);
> >> ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
> >> datasrc/ueventd/ueventd.h:70:69: note: expanded from macro 'rd_info'
> >> #define rd_info(format, arg...) rd_log_message(LOG_INFO, format, ##arg)
> >> ~~~~~~    ^
> >> 2 warnings generated.
> >>
> >> то есть как видишь у clang PRIu64 превратилось в пустую строку и 
> >> аргументы
> >> съехали.
> >>
> >> Таким образом использование PRIu64 сразу делает код gcc-specific.
> >
> > Предполагаю, что дело в отсутствии определения:
> >
> > #ifndef __STDC_FORMAT_MACROS
> > #define __STDC_FORMAT_MACROS 1
> > #endif
> >
> > #include <inttypes.h>
> >
> > поскольку эти PR... определения конфликтуют с чем-то из c11.
> >
> 
> Так и есть. Вложил пример, который проверил с clang-15, работает...

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

В lzma.h есть вот такой комментарий:

/* Use the standard inttypes.h. */
#ifdef __cplusplus
	/*
	 * C99 sections 7.18.2 and 7.18.4 specify
	 * that C++ implementations define the limit
	 * and constant macros only if specifically
	 * requested. Note that if you want the
	 * format macros (PRIu64 etc.) too, you need
	 * to define __STDC_FORMAT_MACROS before
	 * including lzma.h, since re-including
	 * inttypes.h with __STDC_FORMAT_MACROS
	 * defined doesn't necessarily work.
	 */
#	ifndef __STDC_LIMIT_MACROS
#		define __STDC_LIMIT_MACROS 1
#	endif
#	ifndef __STDC_CONSTANT_MACROS
#		define __STDC_CONSTANT_MACROS 1
#	endif
#endif
#include <inttypes.h>

> #define _GNU_SOURCE
> 
> #ifndef __STDC_FORMAT_MACROS
> #define __STDC_FORMAT_MACROS 1
> #endif
> 
> #include <inttypes.h>
> #include <stdio.h>
> 
> 
> int main() {
> 	uint64_t v = UINT64_C(12345);
> 
> 	printf("Value is: %" PRIu64 "\n", v);
> 
> 	return 0;
> }

Я не против добавить нечто подобное коду из lzma.h в ueventd.h, хотя это
выгладит странно.

-- 
Rgrds, legion