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

[Olivier Brunel]

On 07/01/14 06:56, Allan McRae wrote:
> 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 assume by prefixes you mean the front-end could add e.g. "warning:"
before the message itself? Then sure, that part would be translated in
the front-end, but I fail to see how getting the message as a string and
not a format string & va_list would change/break anything?

Either way, the front-end has no idea what the message is, nor does it
care/do anything other than showing it to the user.

(And pm_asprintf could simply be moved to util-common.c no?)

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

I feel it's a little sad there isn't a way to silence this
false-positive warning, since this is really all it's about.

I guess the log already had its own callback separated from events,
although looking at the history it seems more to be because the log_cb
was created first than for any actual reason, and "merging it" into
events was a good idea IMO.

(Of course, if there's ever another event with such a generated
string/message, it'll have to be an event, and that'll make the log
stand out even more for no good reason...)

I just think making it an event was a good thing, but I don't feel very
strongly about this, so I guess you could just revert to a separate log
callback -- though I'm curious what's the main motivation for favoring
this solution? Avoiding the malloc/free calls (A buffer could also be
used/printed into then, leaving the malloc only for extra-long messages) ?

-j


> 
> Allan
>