qemu: command: Report stderr from qemu-bridge-helper

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(-)

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" :-).

> +            cmdstr, virStrerror(errno, ebuf, sizeof(ebuf)),
> +            errstr ? errstr : "");
> +        VIR_FREE(errstr);
>           goto cleanup;
>       }
>   
> @@ -333,6 +351,8 @@ static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
>       }
>   
>    cleanup:
> +    VIR_FREE(cmdstr);
> +    VIR_FREE(errbuf);
>       virCommandFree(cmd);
>       VIR_FORCE_CLOSE(pair[0]);
>       return *tapfd < 0 ? -1 : 0;

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list