Lars Schneider <larsxschneider@xxxxxxxxx> writes:
>> On 15 Aug 2017, at 19:36, Christian Couder <christian.couder@xxxxxxxxx> wrote:
>>
>> In handshake_capabilities() we use warning() when a capability
>> is not supported, so the exit code of the function is 0 and no
>> further error is shown. This is a problem because the warning
>> message doesn't tell us which subprocess cmd failed.
>>
>> On the contrary if we cannot write a packet from this function,
>> we use error() and then subprocess_start() outputs:
>>
>> initialization for subprocess '<cmd>' failed
>>
>> so we can know which subprocess cmd failed.
>>
>> Let's improve the warning() message, so that we can know which
>> subprocess cmd failed.
>>
>> Signed-off-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
>> ---
>> sub-process.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/sub-process.c b/sub-process.c
>> index 6edb97c1c6..6b133f8dce 100644
>> --- a/sub-process.c
>> +++ b/sub-process.c
>> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,
>>
>> static int handshake_capabilities(struct child_process *process,
>> struct subprocess_capability *capabilities,
>> - unsigned int *supported_capabilities)
>> + unsigned int *supported_capabilities,
>> + const char *cmd)
>> {
>> int i;
>> char *line;
>> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,
>> if (supported_capabilities)
>> *supported_capabilities |= capabilities[i].flag;
>> } else {
>> - warning("external filter requested unsupported filter capability '%s'",
>> - p);
>> + warning("subprocess '%s' requested unsupported capability '%s'",
>> + cmd, p);
>
> Wouldn't it be possible to use "process->argv[0]"?
> Shouldn't that be the same as "cmd"?
It is good to see many people are in agreement and in favor of
giving more information. It is even better to see somebody noticing
a room for improvement ;-)
I also wonder if this should be left as a silent warning that the
caller cannot notice as in the corrent code. Dying here might not
be desirable for some callers, but even if we don't die here, we may
want to give the caller a chance to react to the protocol error.
