[libvirt] [PATCH] qemu: command: Report stderr from qemu-bridge-helper

[Cole Robinson]

On 09/10/2015 02:37 PM, Laine Stump wrote:
> On 09/10/2015 12:35 PM, Cole Robinson wrote:
>> There's a couple reports of things failing in this area (bug 1259070),
>> but it's tough to tell what's going wrong without stderr from
>> qemu-bridge-helper. So let's report stderr in the error message
>>
>> Couple new examples:
>>
>> virbr0 is inactive:
>> internal error: /usr/libexec/qemu-bridge-helper --use-vnet --br=virbr0
>> --fd=21: failed to retrieve file descriptor: Transport endpoint is not
>> connected
>> stderr=failed to get mtu of bridge `virbr0': No such device
>>
>> bridge isn't on the ACL:
>> internal error: /usr/libexec/qemu-bridge-helper --use-vnet --br=br0 --fd=21:
>> failed to retrieve file descriptor: Transport endpoint is not connected
>> stderr=access denied by acl file
>> ---
>>   src/qemu/qemu_command.c | 24 ++++++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index ec5e3d4..fc22f36 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -285,6 +285,7 @@ static int
>> qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
>>                                               unsigned int flags)
>>   {
>>       virCommandPtr cmd;
>> +    char *errbuf = NULL, *cmdstr = NULL;
>>       int pair[2] = { -1, -1 };
>>         if ((flags & ~VIR_NETDEV_TAP_CREATE_VNET_HDR) !=
>> VIR_NETDEV_TAP_CREATE_IFUP)
>> @@ -306,6 +307,8 @@ static int
>> qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
>>           virCommandAddArgFormat(cmd, "--use-vnet");
>>       virCommandAddArgFormat(cmd, "--br=%s", brname);
>>       virCommandAddArgFormat(cmd, "--fd=%d", pair[1]);
>> +    virCommandSetErrorBuffer(cmd, &errbuf);
>> +    virCommandDoAsyncIO(cmd);
>>       virCommandPassFD(cmd, pair[1],
>>                        VIR_COMMAND_PASS_FD_CLOSE_PARENT);
>>       virCommandClearCaps(cmd);
>> @@ -320,9 +323,24 @@ static int
>> qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
>>       do {
>>           *tapfd = recvfd(pair[0], 0);
>>       } while (*tapfd < 0 && errno == EINTR);
>> +
>>       if (*tapfd < 0) {
>> -        virReportSystemError(errno, "%s",
>> -                             _("failed to retrieve file descriptor for
>> interface"));
>> +        char ebuf[1024];
> 
> I dislike using up stack space for these, but you're not the first who's done
> it (and it would be most unfortunate to encounter another error trying to
> allocate the buffer in order to report the first error).
> 
>> +        char *errstr = NULL;
>> +
>> +        if (!(cmdstr = virCommandToString(cmd)))
>> +            goto cleanup;
>> +        virCommandAbort(cmd);
>> +
>> +        if (errbuf && *errbuf &&
>> +            virAsprintf(&errstr, "\nstderr=%s", errbuf) < 0)
>> +            goto cleanup;
>> +
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +            _("%s: failed to retrieve file descriptor: %s%s"),
> 
> Maybe this could say something a bit less cryptic/confusing than "failed to
> retrieve file descriptor", e.g. "failed to setup connection to bridge device";
> I know it's technically correct, but may lead the user in the wrong
> direction). Otherwise a very useful/welcome change!
> 
> ACK with a rewording of the message (not necessarily my rewording, but just
> "something" :-).
> 

I changed it to 'failed to communicate with bridge helper'. Thanks for the
review, pushed now

- Cole