[pacman-dev] [PATCH v2 2/2] Update the question callback

[Allan McRae]

On 29/06/14 21:35, Olivier Brunel wrote:
> On 06/29/14 03:50, Allan McRae wrote:
>> On 24/06/14 20:18, Olivier Brunel wrote:
>>> On 06/24/14 08:14, Allan McRae wrote:
>>>> On 26/05/14 01:30, Andrew Gregory wrote:
>>>>> On 05/24/14 at 03:49pm, Olivier Brunel wrote:
>>>>>> Much like with events, instead of using a bunch of void* arguments for
>>>>>> all questions, we now send one pointer to an alpm_question_t struct.
>>>>>> This contains the type of question that was triggered.
>>>>>>
>>>>>> With this information, the pointer can then be typecasted to the
>>>>>> question-specific struct in order to get additional arguments.
>>>>>>
>>>>>> Signed-off-by: Olivier Brunel <jjk at jjacky.com>
>>>>>> ---
>>>>>>  lib/libalpm/alpm.h    |  92 +++++++++++++++++++++++++++++++++++++++--
>>>>>>  lib/libalpm/deps.c    |  38 ++++++++++-------
>>>>>>  lib/libalpm/handle.h  |   4 +-
>>>>>>  lib/libalpm/signing.c |  12 ++++--
>>>>>>  lib/libalpm/sync.c    |  60 +++++++++++++++++----------
>>>>>>  src/pacman/callback.c | 110 +++++++++++++++++++++++++++-----------------------
>>>>>>  src/pacman/callback.h |   3 +-
>>>>>>  7 files changed, 222 insertions(+), 97 deletions(-)
>>>>>
>>>>> The generic to specific struct casts cause Clang to fail when pacman
>>>>> is configured with --enable-warningflags:
>>>>>
>>>>>  callback.c:377:44: error: cast from 'alpm_question_t *' (aka 'struct
>>>>>        _alpm_question_t *') to 'alpm_question_install_ignorepkg_t *'
>>>>>        (aka 'struct _alpm_question_install_ignorepkg_t *') increases
>>>>>        required alignment from 4 to 8 [-Werror,-Wcast-align]
>>>>>    ...*q = (alpm_question_install_ignorepkg_t *) question;
>>>>>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>
>>>>
>>>> These are silenced.
>>>>
>>>>> The earlier changes to the event callback have the same issue and also
>>>>> give errors about using non-literal strings with vprintf:
>>>>>
>>>>>  testdb.c:53:11: error: format string is not a string literal
>>>>>        [-Werror,-Wformat-nonliteral]
>>>>>                  vprintf(event->fmt, event->args);
>>>>>                          ^~~~~~~~~~
>>>>
>>>>
>>>> BAH!   Why did gcc no see this?
>>>>
>>>> Anyway, possible ways to fix this:
>>>>
>>>> 1)
>>>> +#pragma GCC diagnostic push
>>>> +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
>>>>                 vprintf(event->fmt, event->args);
>>>> +#pragma GCC diagnostic pop
>>>>
>>>> This is crap because all clients that want to print a log message will
>>>> need to wrap their calls like this.
>>>>
>>>> 2)
>>>> Revert that change that made the logging an event.
>>>>
>>>>
>>>> Unless someone comes up with something else, I'll do the revert soon.
>>>
>>> I can't think of another way to silence the warning, but there's a way
>>> to "work around" it: instead of putting fmt & args inside
>>> alpm_event_log_t, we put the resulting string (i.e. call pm_asprintf()
>>> in alpm and send the resulting string in the event).
>>>
>>> No more false warnings then (but a malloc/free for each log event). I
>>> can send a patch if you'd like.
>>>
>>
>> Can not do that.   Translations of messages happen in the front-end.
> 
> um, no? Translations obviously happen in ALPM, the call to gettext
> happens before calling _alpm_log(), so by then whether we send the
> message to the front-end as a pointer to the full string, or a pointer
> to a format string and a va_list, doesn't really matter.
> 
> All the front-end does is decide whether or not (e.g. based on level),
> and how, to show the message to the user.
> 
> Or did I not understand what you meant at all?
> 

Some parts are, some are not (the prefixes).  Also, pm_asprintf is not
available in the backend.

I am in favour of reverting the log callback as the solution here.

Allan