[libvirt] [PATCH 3/7] qemu: eliminate almost-duplicate code in qemu_command.c

[Laine Stump]

On 08/04/2013 08:09 PM, Doug Goldstein wrote:
> On Fri, Aug 2, 2013 at 11:55 AM, Laine Stump <laine at laine.org> wrote:
>> - * Check that the PCI address is valid for use
>> - * with the specified PCI address set.
>> +static bool
>> +qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr,
>> +                                    qemuDomainPCIConnectFlags busFlags,
>> +                                    qemuDomainPCIConnectFlags devFlags,
>> +                                    bool reportError)
>> +{
>> +    /* If this bus doesn't allow the type of connection (PCI
>> +     * vs. PCIe) required by the device, or if the device requires
>> +     * hot-plug and this bus doesn't have it, return false.
>> +     */
>> +    if (!(devFlags & busFlags & QEMU_PCI_CONNECT_TYPES_MASK)) {
>> +        if (reportError) {
>> +            if (devFlags & QEMU_PCI_CONNECT_TYPE_PCI) {
>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                               _("PCI bus %.4x:%.2x is not compatible with the "
>> +                                 "device. Device requires a standard PCI slot, "
>> +                                 "which is not provided by this bus"),
>> +                               addr->domain, addr->bus);
> So even though this patch has been ACK'd and committed. I really would
> have liked to see some info about the device printed in the error
> message because when you're defining a domain its really not clear
> which device is the problem.

Yes, that would require some extra plumbing and adding in extra code
because this is a common function called for many different types of
device, so we would need to pass in a device-type enum. If, on the other
hand, we just send an error code back up the return stack to the caller,
then we'll have a dozen or so callers with nearly (but not quite)
identical error messages.

>
>> +            } else {
>> +                /* this should never happen. If it does, there is a
>> +                 * bug in the code that sets the flag bits for devices.
>> +                 */
>> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                           _("The device information has no PCI "
>> +                             "connection types listed"));
>> +            }
>> +        }
>> +        return false;
>> +    }
>> +    if ((devFlags & QEMU_PCI_CONNECT_HOTPLUGGABLE) &&
>> +        !(busFlags & QEMU_PCI_CONNECT_HOTPLUGGABLE)) {
>> +        if (reportError) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("PCI bus %.4x:%.2x is not compatible with the "
>> +                             "device. Device requires hot-plug capability, "
>> +                             "which is not provided by the bus"),
>> +                           addr->domain, addr->bus);
> Same as above, when you're defining the whole domain its really not
> clear which device is the problem so provide some details on which one
> it is.

Same issue makes it cumbersome - the more detailed info about the type
of device (and which device of that type) is several layers up the call
chain, so it's going to take extra setup at the higher level, and
passing a device-type enum down to do this.

I agree that it needs to be done before 1.1.2, but I think it's
important for this code to get wider testing even sooner.