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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list