5:08 a.m.
The call to qemuBuildDeviceAddressStr() happens no matter
what, so we can move it to the outer possible scope inside
the function.
We can also move the call to virBufferAsprintf() after all
the checks have been performed, where it makes more sense.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_command.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c53ab97..5118541 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10292,14 +10292,8 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
virBufferAsprintf(&cmd, "spapr-vty,chardev=char%s",
serial->info.alias);
- if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
- goto error;
}
} else {
- virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
- virDomainChrSerialTargetTypeToString(serial->targetType),
- serial->info.alias, serial->info.alias);
-
switch (serial->targetType) {
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_SERIAL)) {
@@ -10314,9 +10308,6 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("usb-serial requires address of usb type"));
goto error;
}
-
- if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
- goto error;
break;
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
@@ -10326,9 +10317,6 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("isa-serial requires address of isa type"));
goto error;
}
-
- if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
- goto error;
break;
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
@@ -10344,13 +10332,17 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("pci-serial requires address of pci type"));
goto error;
}
-
- if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
- goto error;
break;
}
+
+ virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
+ virDomainChrSerialTargetTypeToString(serial->targetType),
+ serial->info.alias, serial->info.alias);
}
+ if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0)
+ goto error;
+
if (virBufferCheckError(&cmd) < 0)
goto error;
--
2.7.5
3:03 p.m.
On 06/22/2017 06:08 AM, Andrea Bolognani wrote:
The call to qemuBuildDeviceAddressStr() happens no matter
what, so we can move it to the outer possible scope inside
the function.
We can also move the call to virBufferAsprintf() after all
the checks have been performed, where it makes more sense.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_command.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c53ab97..5118541 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10292,14 +10292,8 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
virBufferAsprintf(&cmd, "spapr-vty,chardev=char%s",
serial->info.alias);
- if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
- goto error;
}
} else {
- virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
- virDomainChrSerialTargetTypeToString(serial->targetType),
- serial->info.alias, serial->info.alias);
-
switch (serial->targetType) {
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_SERIAL)) {
@@ -10314,9 +10308,6 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("usb-serial requires address of usb type"));
goto error;
}
-
- if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
- goto error;
break;
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
@@ -10326,9 +10317,6 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("isa-serial requires address of isa type"));
goto error;
}
-
- if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
- goto error;
break;
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
@@ -10344,13 +10332,17 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("pci-serial requires address of pci type"));
goto error;
}
-
- if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
- goto error;
break;
}
+
+ virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
+ virDomainChrSerialTargetTypeToString(serial->targetType),
+ serial->info.alias, serial->info.alias);
}
+ if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) <
0)
+ goto error;
+
I haven't looked at the caller of qemuBuildSerialChrDeviceStr() with a
fine-toothed comb, but this patch does change the code a bit. In
particular, previously if qemuDomainIsPSeries(def) was true, but either
one of these was false:
serial->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL
serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO
then it would return an empty string in *deviceStr. But now it will
instead return an address string, without the
"spapr-vty,chardev=charBLAH" at the beginning. So can we guarantee that
the above two conditions are always true for PSeries?
I guess in both the "before" and "after" cases, the result would be a
failure (with before, we would end up with a "-device" with nothing
following it, and now we would have "-device" with an address string but
none of the preceding stuff describing the address).
(Similarly, before this patch, if serial->targetType wasn't one of
VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE(USB|ISA|PCI) then *deviceStr would
contain "(null),chardev=charBLAH,id=BLAH" with no address, but now it
will have an address as well as the nonsensical preamble. (yes, I'm
being ridiculous in this case :-P))
In the end, ACK to your patch since you haven't made the behavior any
worse (and have eliminated a bunch of duplicated code, but I do think
that either the checks on deviceType and info.type should be removed as
pointless, or that they should trigger a failure directly rather than
just creating a bad commandline.
if (virBufferCheckError(&cmd) < 0)
goto error;
11:37 a.m.
On Thu, 2017-06-22 at 16:03 -0400, Laine Stump wrote:
> The call to qemuBuildDeviceAddressStr() happens no matter
> what, so we can move it to the outer possible scope inside
> the function.
>
> We can also move the call to virBufferAsprintf() after all
> the checks have been performed, where it makes more sense.
>
> Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 22 +++++++---------------
> 1 file changed, 7 insertions(+), 15 deletions(-)
Picking up old stuff...
I haven't looked at the caller of qemuBuildSerialChrDeviceStr()
with a
fine-toothed comb, but this patch does change the code a bit. In
particular, previously if qemuDomainIsPSeries(def) was true, but either
one of these was false:
serial->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL
serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO
then it would return an empty string in *deviceStr. But now it will
instead return an address string, without the
"spapr-vty,chardev=charBLAH" at the beginning. So can we guarantee that
the above two conditions are always true for PSeries?
The check on serial->deviceType is pointless, because we're
only calling this function for TYPE_SERIAL. I'm pretty sure
the latter can't be false because...
I guess in both the "before" and "after" cases,
the result would be a
failure (with before, we would end up with a "-device" with nothing
following it, and now we would have "-device" with an address string but
none of the preceding stuff describing the address).
... otherwise we would have run into this. But I'll check.
(Similarly, before this patch, if serial->targetType wasn't
one of
VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE(USB|ISA|PCI) then *deviceStr would
contain "(null),chardev=charBLAH,id=BLAH" with no address, but now it
will have an address as well as the nonsensical preamble. (yes, I'm
being ridiculous in this case :-P))
The only other case would be TYPE_LAST, which hopefully
will never happen. I can add a check for it though.
In the end, ACK to your patch since you haven't made the
behavior any
worse (and have eliminated a bunch of duplicated code, but I do think
that either the checks on deviceType and info.type should be removed as
pointless, or that they should trigger a failure directly rather than
just creating a bad commandline.
I think we can go further and unify the way things are
handled so that pSeries doesn't need to be special-cased.
I've pushed the patch as it is for the moment, based on
your ACK, and will post a follow-up shortly.
--
Andrea Bolognani / Red Hat / Virtualization
2711
days inactive
2740
days old
2 comments
2 participants
participants (2)
-
Andrea Bolognani -
Laine Stump