[libvirt] [PATCH REPOST 0/9] Reorganize qemu_command.c in smaller piles

Rather than hope for a review of 25 patches or creating some remote place to place a branch - I'll repost the original: http://www.redhat.com/archives/libvir-list/2016-February/msg00975.html in smaller chunks. No differences in these patches other than a rebase to top of tree a short while ago (commit id 'cf091094a'). There were also no conflicts from changes since initial posting. John Ferlan (9): qemu: Introduce qemuBuildClockCommandLine qemu: Introduce qemuBuildPMCommandLine qemu: Introduce qemuBuildBootCommandLine qemu: Introduce qemuBuildGlobalControllerCommandLine qemu: Introduce qemuBuildControllerDevCommandLine qemu: Introduce qemuBuildHubCommandLine qemu: Introduce qemuBuildDiskDriveCommandLine qemu: Introduce qemuBuildFSDevCommandLine qemu: Introduce qemuBuildNetCommandLine src/conf/domain_conf.c | 14 +- src/conf/domain_conf.h | 8 +- src/qemu/qemu_command.c | 5488 +++++++++++++++++++++------------------- src/qemu/qemu_command.h | 15 +- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_domain_address.h | 2 +- 6 files changed, 2843 insertions(+), 2686 deletions(-) -- 2.5.0

Add new function to manage adding the '-clock' options to the command line removing that task from the mainline qemuBuildCommandLine. Also includes some minor formatting cleanups. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 300 ++++++++++++++++++++++++++---------------------- 1 file changed, 160 insertions(+), 140 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 000c29d..c890b75 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4621,6 +4621,7 @@ qemuBuildSgaCommandLine(virCommandPtr cmd, static char * qemuBuildClockArgStr(virDomainClockDefPtr def) { + size_t i; virBuffer buf = VIR_BUFFER_INITIALIZER; switch (def->offset) { @@ -4685,8 +4686,8 @@ qemuBuildClockArgStr(virDomainClockDefPtr def) goto error; } - /* Look for an 'rtc' timer element, and add in appropriate clock= and driftfix= */ - size_t i; + /* Look for an 'rtc' timer element, and add in appropriate + * clock= and driftfix= */ for (i = 0; i < def->ntimers; i++) { if (def->timers[i]->name == VIR_DOMAIN_TIMER_NAME_RTC) { switch (def->timers[i]->track) { @@ -4736,6 +4737,161 @@ qemuBuildClockArgStr(virDomainClockDefPtr def) return NULL; } + +/* NOTE: Building of commands can change def->clock->data.* values, so + * virDomainDef is not const here. + */ +static int +qemuBuildClockCommandLine(virCommandPtr cmd, + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) +{ + size_t i; + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_RTC)) { + char *rtcopt; + virCommandAddArg(cmd, "-rtc"); + if (!(rtcopt = qemuBuildClockArgStr(&def->clock))) + return -1; + virCommandAddArg(cmd, rtcopt); + VIR_FREE(rtcopt); + } else { + switch (def->clock.offset) { + case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: + case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE: + virCommandAddArg(cmd, "-localtime"); + break; + + case VIR_DOMAIN_CLOCK_OFFSET_UTC: + /* Nothing, its the default */ + break; + + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported clock offset '%s'"), + virDomainClockOffsetTypeToString(def->clock.offset)); + return -1; + } + } + + if (def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE && + def->clock.data.timezone) { + virCommandAddEnvPair(cmd, "TZ", def->clock.data.timezone); + } + + for (i = 0; i < def->clock.ntimers; i++) { + switch ((virDomainTimerNameType) def->clock.timers[i]->name) { + case VIR_DOMAIN_TIMER_NAME_PLATFORM: + case VIR_DOMAIN_TIMER_NAME_TSC: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported timer type (name) '%s'"), + virDomainTimerNameTypeToString(def->clock.timers[i]->name)); + return -1; + + case VIR_DOMAIN_TIMER_NAME_KVMCLOCK: + case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK: + /* Timers above are handled when building -cpu. */ + case VIR_DOMAIN_TIMER_NAME_LAST: + break; + + case VIR_DOMAIN_TIMER_NAME_RTC: + /* This has already been taken care of (in qemuBuildClockArgStr) + if QEMU_CAPS_RTC is set (mutually exclusive with + QEMUD_FLAG_RTC_TD_HACK) */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_RTC_TD_HACK)) { + switch (def->clock.timers[i]->tickpolicy) { + case -1: + case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY: + /* the default - do nothing */ + break; + case VIR_DOMAIN_TIMER_TICKPOLICY_CATCHUP: + virCommandAddArg(cmd, "-rtc-td-hack"); + break; + case VIR_DOMAIN_TIMER_TICKPOLICY_MERGE: + case VIR_DOMAIN_TIMER_TICKPOLICY_DISCARD: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported rtc tickpolicy '%s'"), + virDomainTimerTickpolicyTypeToString(def->clock.timers[i]->tickpolicy)); + return -1; + } + } else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_RTC) && + (def->clock.timers[i]->tickpolicy + != VIR_DOMAIN_TIMER_TICKPOLICY_DELAY) && + (def->clock.timers[i]->tickpolicy != -1)) { + /* a non-default rtc policy was given, but there is no + way to implement it in this version of qemu */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported rtc tickpolicy '%s'"), + virDomainTimerTickpolicyTypeToString(def->clock.timers[i]->tickpolicy)); + return -1; + } + break; + + case VIR_DOMAIN_TIMER_NAME_PIT: + switch (def->clock.timers[i]->tickpolicy) { + case -1: + case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY: + /* delay is the default if we don't have kernel + (-no-kvm-pit), otherwise, the default is catchup. */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM_PIT_TICK_POLICY)) + virCommandAddArgList(cmd, "-global", + "kvm-pit.lost_tick_policy=discard", NULL); + else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_KVM_PIT)) + virCommandAddArg(cmd, "-no-kvm-pit-reinjection"); + break; + case VIR_DOMAIN_TIMER_TICKPOLICY_CATCHUP: + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_KVM_PIT) || + virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM_PIT_TICK_POLICY)) { + /* do nothing - this is default for kvm-pit */ + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDF)) { + /* -tdf switches to 'catchup' with userspace pit. */ + virCommandAddArg(cmd, "-tdf"); + } else { + /* can't catchup if we have neither pit mode */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported pit tickpolicy '%s'"), + virDomainTimerTickpolicyTypeToString(def->clock.timers[i]->tickpolicy)); + return -1; + } + break; + case VIR_DOMAIN_TIMER_TICKPOLICY_MERGE: + case VIR_DOMAIN_TIMER_TICKPOLICY_DISCARD: + /* no way to support these modes for pit in qemu */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported pit tickpolicy '%s'"), + virDomainTimerTickpolicyTypeToString(def->clock.timers[i]->tickpolicy)); + return -1; + } + break; + + case VIR_DOMAIN_TIMER_NAME_HPET: + /* the only meaningful attribute for hpet is "present". If + * present is -1, that means it wasn't specified, and + * should be left at the default for the + * hypervisor. "default" when -no-hpet exists is "yes", + * and when -no-hpet doesn't exist is "no". "confusing"? + * "yes"! */ + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_HPET)) { + if (def->clock.timers[i]->present == 0) + virCommandAddArg(cmd, "-no-hpet"); + } else { + /* no hpet timer available. The only possible action + is to raise an error if present="yes" */ + if (def->clock.timers[i]->present == 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("hpet timer is not supported")); + return -1; + } + } + break; + } + } + + return 0; +} + + static int qemuBuildCpuModelArgStr(virQEMUDriverPtr driver, const virDomainDef *def, @@ -7182,144 +7338,8 @@ qemuBuildCommandLine(virConnectPtr conn, monitor_json) < 0) goto error; - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_RTC)) { - char *rtcopt; - virCommandAddArg(cmd, "-rtc"); - if (!(rtcopt = qemuBuildClockArgStr(&def->clock))) - goto error; - virCommandAddArg(cmd, rtcopt); - VIR_FREE(rtcopt); - } else { - switch (def->clock.offset) { - case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: - case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE: - virCommandAddArg(cmd, "-localtime"); - break; - - case VIR_DOMAIN_CLOCK_OFFSET_UTC: - /* Nothing, its the default */ - break; - - default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported clock offset '%s'"), - virDomainClockOffsetTypeToString(def->clock.offset)); - goto error; - } - } - if (def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE && - def->clock.data.timezone) { - virCommandAddEnvPair(cmd, "TZ", def->clock.data.timezone); - } - - for (i = 0; i < def->clock.ntimers; i++) { - switch ((virDomainTimerNameType) def->clock.timers[i]->name) { - case VIR_DOMAIN_TIMER_NAME_PLATFORM: - case VIR_DOMAIN_TIMER_NAME_TSC: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported timer type (name) '%s'"), - virDomainTimerNameTypeToString(def->clock.timers[i]->name)); - goto error; - - case VIR_DOMAIN_TIMER_NAME_KVMCLOCK: - case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK: - /* Timers above are handled when building -cpu. */ - case VIR_DOMAIN_TIMER_NAME_LAST: - break; - - case VIR_DOMAIN_TIMER_NAME_RTC: - /* This has already been taken care of (in qemuBuildClockArgStr) - if QEMU_CAPS_RTC is set (mutually exclusive with - QEMUD_FLAG_RTC_TD_HACK) */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_RTC_TD_HACK)) { - switch (def->clock.timers[i]->tickpolicy) { - case -1: - case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY: - /* the default - do nothing */ - break; - case VIR_DOMAIN_TIMER_TICKPOLICY_CATCHUP: - virCommandAddArg(cmd, "-rtc-td-hack"); - break; - case VIR_DOMAIN_TIMER_TICKPOLICY_MERGE: - case VIR_DOMAIN_TIMER_TICKPOLICY_DISCARD: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported rtc tickpolicy '%s'"), - virDomainTimerTickpolicyTypeToString(def->clock.timers[i]->tickpolicy)); - goto error; - } - } else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_RTC) && - (def->clock.timers[i]->tickpolicy - != VIR_DOMAIN_TIMER_TICKPOLICY_DELAY) && - (def->clock.timers[i]->tickpolicy != -1)) { - /* a non-default rtc policy was given, but there is no - way to implement it in this version of qemu */ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported rtc tickpolicy '%s'"), - virDomainTimerTickpolicyTypeToString(def->clock.timers[i]->tickpolicy)); - goto error; - } - break; - - case VIR_DOMAIN_TIMER_NAME_PIT: - switch (def->clock.timers[i]->tickpolicy) { - case -1: - case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY: - /* delay is the default if we don't have kernel - (-no-kvm-pit), otherwise, the default is catchup. */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM_PIT_TICK_POLICY)) - virCommandAddArgList(cmd, "-global", - "kvm-pit.lost_tick_policy=discard", NULL); - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_KVM_PIT)) - virCommandAddArg(cmd, "-no-kvm-pit-reinjection"); - break; - case VIR_DOMAIN_TIMER_TICKPOLICY_CATCHUP: - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_KVM_PIT) || - virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM_PIT_TICK_POLICY)) { - /* do nothing - this is default for kvm-pit */ - } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDF)) { - /* -tdf switches to 'catchup' with userspace pit. */ - virCommandAddArg(cmd, "-tdf"); - } else { - /* can't catchup if we have neither pit mode */ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported pit tickpolicy '%s'"), - virDomainTimerTickpolicyTypeToString(def->clock.timers[i]->tickpolicy)); - goto error; - } - break; - case VIR_DOMAIN_TIMER_TICKPOLICY_MERGE: - case VIR_DOMAIN_TIMER_TICKPOLICY_DISCARD: - /* no way to support these modes for pit in qemu */ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported pit tickpolicy '%s'"), - virDomainTimerTickpolicyTypeToString(def->clock.timers[i]->tickpolicy)); - goto error; - } - break; - - case VIR_DOMAIN_TIMER_NAME_HPET: - /* the only meaningful attribute for hpet is "present". If - * present is -1, that means it wasn't specified, and - * should be left at the default for the - * hypervisor. "default" when -no-hpet exists is "yes", - * and when -no-hpet doesn't exist is "no". "confusing"? - * "yes"! */ - - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_HPET)) { - if (def->clock.timers[i]->present == 0) - virCommandAddArg(cmd, "-no-hpet"); - } else { - /* no hpet timer available. The only possible action - is to raise an error if present="yes" */ - if (def->clock.timers[i]->present == 1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("hpet timer is not supported")); - goto error; - } - } - break; - } - } + if (qemuBuildClockCommandLine(cmd, def, qemuCaps) < 0) + goto error; /* Only add -no-reboot option if each event destroys domain */ if (def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY && -- 2.5.0

On 03/08/2016 02:03 PM, John Ferlan wrote:
Add new function to manage adding the '-clock' options to the command line removing that task from the mainline qemuBuildCommandLine.
Also includes some minor formatting cleanups.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 300 ++++++++++++++++++++++++++---------------------- 1 file changed, 160 insertions(+), 140 deletions(-)
ACK - Cole

Add new function to manage adding the power management options to the command line removing that task from the mainline qemuBuildCommandLine. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 136 ++++++++++++++++++++++++++---------------------- 1 file changed, 75 insertions(+), 61 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c890b75..3b3c958 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4893,6 +4893,79 @@ qemuBuildClockCommandLine(virCommandPtr cmd, static int +qemuBuildPMCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps, + bool monitor_json) +{ + bool allowReboot = true; + + /* Only add -no-reboot option if each event destroys domain */ + if (def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY && + def->onPoweroff == VIR_DOMAIN_LIFECYCLE_DESTROY && + (def->onCrash == VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY || + def->onCrash == VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_DESTROY)) { + allowReboot = false; + virCommandAddArg(cmd, "-no-reboot"); + } + + /* If JSON monitor is enabled, we can receive an event + * when QEMU stops. If we use no-shutdown, then we can + * watch for this event and do a soft/warm reboot. + */ + if (monitor_json && allowReboot && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) { + virCommandAddArg(cmd, "-no-shutdown"); + } + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_ACPI)) { + if (def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON) + virCommandAddArg(cmd, "-no-acpi"); + } + + /* We fall back to PIIX4_PM even for q35, since it's what we did + pre-q35-pm support. QEMU starts up fine (with a warning) if + mixing PIIX PM and -M q35. Starting to reject things here + could mean we refuse to start existing configs in the wild.*/ + if (def->pm.s3) { + const char *pm_object = "PIIX4_PM"; + + if (qemuDomainMachineIsQ35(def) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_DISABLE_S3)) { + pm_object = "ICH9-LPC"; + } else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S3)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("setting ACPI S3 not supported")); + return -1; + } + + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "%s.disable_s3=%d", + pm_object, def->pm.s3 == VIR_TRISTATE_BOOL_NO); + } + + if (def->pm.s4) { + const char *pm_object = "PIIX4_PM"; + + if (qemuDomainMachineIsQ35(def) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_DISABLE_S4)) { + pm_object = "ICH9-LPC"; + } else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S4)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("setting ACPI S4 not supported")); + return -1; + } + + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "%s.disable_s4=%d", + pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO); + } + + return 0; +} + + +static int qemuBuildCpuModelArgStr(virQEMUDriverPtr driver, const virDomainDef *def, virBufferPtr buf, @@ -7204,7 +7277,6 @@ qemuBuildCommandLine(virConnectPtr conn, bool havespice = false; int last_good_net = -1; virCommandPtr cmd = NULL; - bool allowReboot = true; bool emitBootindex = false; int usbcontroller = 0; int actualSerials = 0; @@ -7341,66 +7413,8 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildClockCommandLine(cmd, def, qemuCaps) < 0) goto error; - /* Only add -no-reboot option if each event destroys domain */ - if (def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY && - def->onPoweroff == VIR_DOMAIN_LIFECYCLE_DESTROY && - (def->onCrash == VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY || - def->onCrash == VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_DESTROY)) { - allowReboot = false; - virCommandAddArg(cmd, "-no-reboot"); - } - - /* If JSON monitor is enabled, we can receive an event - * when QEMU stops. If we use no-shutdown, then we can - * watch for this event and do a soft/warm reboot. - */ - if (monitor_json && allowReboot && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) { - virCommandAddArg(cmd, "-no-shutdown"); - } - - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_ACPI)) { - if (def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON) - virCommandAddArg(cmd, "-no-acpi"); - } - - /* We fall back to PIIX4_PM even for q35, since it's what we did - pre-q35-pm support. QEMU starts up fine (with a warning) if - mixing PIIX PM and -M q35. Starting to reject things here - could mean we refuse to start existing configs in the wild.*/ - if (def->pm.s3) { - const char *pm_object = "PIIX4_PM"; - - if (qemuDomainMachineIsQ35(def) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_DISABLE_S3)) { - pm_object = "ICH9-LPC"; - } else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S3)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("setting ACPI S3 not supported")); - goto error; - } - - virCommandAddArg(cmd, "-global"); - virCommandAddArgFormat(cmd, "%s.disable_s3=%d", - pm_object, def->pm.s3 == VIR_TRISTATE_BOOL_NO); - } - - if (def->pm.s4) { - const char *pm_object = "PIIX4_PM"; - - if (qemuDomainMachineIsQ35(def) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_DISABLE_S4)) { - pm_object = "ICH9-LPC"; - } else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S4)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("setting ACPI S4 not supported")); - goto error; - } - - virCommandAddArg(cmd, "-global"); - virCommandAddArgFormat(cmd, "%s.disable_s4=%d", - pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO); - } + if (qemuBuildPMCommandLine(cmd, def, qemuCaps, monitor_json) < 0) + goto error; /* * We prefer using explicit bootindex=N parameters for predictable -- 2.5.0

On 03/08/2016 02:03 PM, John Ferlan wrote:
Add new function to manage adding the power management options to the command line removing that task from the mainline qemuBuildCommandLine.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 136 ++++++++++++++++++++++++++---------------------- 1 file changed, 75 insertions(+), 61 deletions(-)
ACK - Cole

Add new function to manage adding the -boot options to the command line removing that task from the mainline qemuBuildCommandLine. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 282 +++++++++++++++++++++++++----------------------- 1 file changed, 150 insertions(+), 132 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3b3c958..2fd91a4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4966,6 +4966,155 @@ qemuBuildPMCommandLine(virCommandPtr cmd, static int +qemuBuildBootCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps, + bool *emitBootindex) +{ + size_t i; + virBuffer boot_buf = VIR_BUFFER_INITIALIZER; + char *boot_order_str = NULL, *boot_opts_str = NULL; + + /* + * We prefer using explicit bootindex=N parameters for predictable + * results even though domain XML doesn't use per device boot elements. + * However, we can't use bootindex if boot menu was requested. + */ + if (!def->os.nBootDevs) { + /* def->os.nBootDevs is guaranteed to be > 0 unless per-device boot + * configuration is used + */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("hypervisor lacks deviceboot feature")); + goto error; + } + *emitBootindex = true; + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX) && + (def->os.bootmenu != VIR_TRISTATE_BOOL_YES || + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_MENU))) { + *emitBootindex = true; + } + + if (!*emitBootindex) { + char boot[VIR_DOMAIN_BOOT_LAST+1]; + + for (i = 0; i < def->os.nBootDevs; i++) { + switch (def->os.bootDevs[i]) { + case VIR_DOMAIN_BOOT_CDROM: + boot[i] = 'd'; + break; + case VIR_DOMAIN_BOOT_FLOPPY: + boot[i] = 'a'; + break; + case VIR_DOMAIN_BOOT_DISK: + boot[i] = 'c'; + break; + case VIR_DOMAIN_BOOT_NET: + boot[i] = 'n'; + break; + default: + boot[i] = 'c'; + break; + } + } + boot[def->os.nBootDevs] = '\0'; + + virBufferAsprintf(&boot_buf, "%s", boot); + if (virBufferCheckError(&boot_buf) < 0) + goto error; + boot_order_str = virBufferContentAndReset(&boot_buf); + } + + if (def->os.bootmenu) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_MENU)) { + if (def->os.bootmenu == VIR_TRISTATE_BOOL_YES) + virBufferAddLit(&boot_buf, "menu=on,"); + else + virBufferAddLit(&boot_buf, "menu=off,"); + } else { + /* We cannot emit an error when bootmenu is enabled but + * unsupported because of backward compatibility */ + VIR_WARN("bootmenu is enabled but not " + "supported by this QEMU binary"); + } + } + + if (def->os.bios.rt_set) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_REBOOT_TIMEOUT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reboot timeout is not supported " + "by this QEMU binary")); + goto error; + } + + virBufferAsprintf(&boot_buf, + "reboot-timeout=%d,", + def->os.bios.rt_delay); + } + + if (def->os.bm_timeout_set) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPLASH_TIMEOUT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("splash timeout is not supported " + "by this QEMU binary")); + goto error; + } + + virBufferAsprintf(&boot_buf, "splash-time=%u,", def->os.bm_timeout); + } + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_STRICT)) + virBufferAddLit(&boot_buf, "strict=on,"); + + virBufferTrim(&boot_buf, ",", -1); + + if (virBufferCheckError(&boot_buf) < 0) + goto error; + + boot_opts_str = virBufferContentAndReset(&boot_buf); + if (boot_order_str || boot_opts_str) { + virCommandAddArg(cmd, "-boot"); + + if (boot_order_str && boot_opts_str) { + virCommandAddArgFormat(cmd, "order=%s,%s", + boot_order_str, boot_opts_str); + } else if (boot_order_str) { + virCommandAddArg(cmd, boot_order_str); + } else if (boot_opts_str) { + virCommandAddArg(cmd, boot_opts_str); + } + } + VIR_FREE(boot_opts_str); + VIR_FREE(boot_order_str); + + if (def->os.kernel) + virCommandAddArgList(cmd, "-kernel", def->os.kernel, NULL); + if (def->os.initrd) + virCommandAddArgList(cmd, "-initrd", def->os.initrd, NULL); + if (def->os.cmdline) + virCommandAddArgList(cmd, "-append", def->os.cmdline, NULL); + if (def->os.dtb) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DTB)) { + virCommandAddArgList(cmd, "-dtb", def->os.dtb, NULL); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dtb is not supported with this QEMU binary")); + goto error; + } + } + + return 0; + + error: + VIR_FREE(boot_order_str); + VIR_FREE(boot_opts_str); + virBufferFreeAndReset(&boot_buf); + return -1; +} + + +static int qemuBuildCpuModelArgStr(virQEMUDriverPtr driver, const virDomainDef *def, virBufferPtr buf, @@ -7306,8 +7455,6 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_DOMAIN_CONTROLLER_TYPE_CCID, }; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - virBuffer boot_buf = VIR_BUFFER_INITIALIZER; - char *boot_order_str = NULL, *boot_opts_str = NULL; virBuffer fdc_opts = VIR_BUFFER_INITIALIZER; char *fdc_opts_str = NULL; int bootCD = 0, bootFloppy = 0, bootDisk = 0, bootHostdevNet = 0; @@ -7416,135 +7563,9 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildPMCommandLine(cmd, def, qemuCaps, monitor_json) < 0) goto error; - /* - * We prefer using explicit bootindex=N parameters for predictable - * results even though domain XML doesn't use per device boot elements. - * However, we can't use bootindex if boot menu was requested. - */ - if (!def->os.nBootDevs) { - /* def->os.nBootDevs is guaranteed to be > 0 unless per-device boot - * configuration is used - */ - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("hypervisor lacks deviceboot feature")); - goto error; - } - emitBootindex = true; - } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX) && - (def->os.bootmenu != VIR_TRISTATE_BOOL_YES || - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_MENU))) { - emitBootindex = true; - } - - if (!emitBootindex) { - char boot[VIR_DOMAIN_BOOT_LAST+1]; - - for (i = 0; i < def->os.nBootDevs; i++) { - switch (def->os.bootDevs[i]) { - case VIR_DOMAIN_BOOT_CDROM: - boot[i] = 'd'; - break; - case VIR_DOMAIN_BOOT_FLOPPY: - boot[i] = 'a'; - break; - case VIR_DOMAIN_BOOT_DISK: - boot[i] = 'c'; - break; - case VIR_DOMAIN_BOOT_NET: - boot[i] = 'n'; - break; - default: - boot[i] = 'c'; - break; - } - } - boot[def->os.nBootDevs] = '\0'; - - virBufferAsprintf(&boot_buf, "%s", boot); - if (virBufferCheckError(&boot_buf) < 0) - goto error; - boot_order_str = virBufferContentAndReset(&boot_buf); - } - - if (def->os.bootmenu) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_MENU)) { - if (def->os.bootmenu == VIR_TRISTATE_BOOL_YES) - virBufferAddLit(&boot_buf, "menu=on,"); - else - virBufferAddLit(&boot_buf, "menu=off,"); - } else { - /* We cannot emit an error when bootmenu is enabled but - * unsupported because of backward compatibility */ - VIR_WARN("bootmenu is enabled but not " - "supported by this QEMU binary"); - } - } - - if (def->os.bios.rt_set) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_REBOOT_TIMEOUT)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("reboot timeout is not supported " - "by this QEMU binary")); - goto error; - } - - virBufferAsprintf(&boot_buf, - "reboot-timeout=%d,", - def->os.bios.rt_delay); - } - - if (def->os.bm_timeout_set) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPLASH_TIMEOUT)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("splash timeout is not supported " - "by this QEMU binary")); - goto error; - } - - virBufferAsprintf(&boot_buf, "splash-time=%u,", def->os.bm_timeout); - } - - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_STRICT)) - virBufferAddLit(&boot_buf, "strict=on,"); - - virBufferTrim(&boot_buf, ",", -1); - - if (virBufferCheckError(&boot_buf) < 0) + if (qemuBuildBootCommandLine(cmd, def, qemuCaps, &emitBootindex) < 0) goto error; - boot_opts_str = virBufferContentAndReset(&boot_buf); - if (boot_order_str || boot_opts_str) { - virCommandAddArg(cmd, "-boot"); - - if (boot_order_str && boot_opts_str) { - virCommandAddArgFormat(cmd, "order=%s,%s", - boot_order_str, boot_opts_str); - } else if (boot_order_str) { - virCommandAddArg(cmd, boot_order_str); - } else if (boot_opts_str) { - virCommandAddArg(cmd, boot_opts_str); - } - } - VIR_FREE(boot_opts_str); - VIR_FREE(boot_order_str); - - if (def->os.kernel) - virCommandAddArgList(cmd, "-kernel", def->os.kernel, NULL); - if (def->os.initrd) - virCommandAddArgList(cmd, "-initrd", def->os.initrd, NULL); - if (def->os.cmdline) - virCommandAddArgList(cmd, "-append", def->os.cmdline, NULL); - if (def->os.dtb) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DTB)) { - virCommandAddArgList(cmd, "-dtb", def->os.dtb, NULL); - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("dtb is not supported with this QEMU binary")); - goto error; - } - } - for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && @@ -8925,9 +8946,6 @@ qemuBuildCommandLine(virConnectPtr conn, return cmd; error: - VIR_FREE(boot_order_str); - VIR_FREE(boot_opts_str); - virBufferFreeAndReset(&boot_buf); virObjectUnref(cfg); /* free up any resources in the network driver * but don't overwrite the original error */ -- 2.5.0

On 03/08/2016 02:03 PM, John Ferlan wrote:
Add new function to manage adding the -boot options to the command line removing that task from the mainline qemuBuildCommandLine.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 282 +++++++++++++++++++++++++----------------------- 1 file changed, 150 insertions(+), 132 deletions(-)
ACK - Cole

Add new function to manage adding the -global controller options to the command line removing that task from the mainline qemuBuildCommandLine. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 106 +++++++++++++++++++++++++++--------------------- 1 file changed, 60 insertions(+), 46 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2fd91a4..87d22f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5115,6 +5115,64 @@ qemuBuildBootCommandLine(virCommandPtr cmd, static int +qemuBuildGlobalControllerCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr cont = def->controllers[i]; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + cont->opts.pciopts.pcihole64) { + const char *hoststr = NULL; + bool cap = false; + bool machine = false; + + switch (cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + hoststr = "i440FX-pcihost"; + cap = virQEMUCapsGet(qemuCaps, QEMU_CAPS_I440FX_PCI_HOLE64_SIZE); + machine = qemuDomainMachineIsI440FX(def); + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + hoststr = "q35-pcihost"; + cap = virQEMUCapsGet(qemuCaps, QEMU_CAPS_Q35_PCI_HOLE64_SIZE); + machine = qemuDomainMachineIsQ35(def); + break; + + default: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("64-bit PCI hole setting is only for root" + " PCI controllers")); + return -1; + } + + if (!machine) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Setting the 64-bit PCI hole size is not " + "supported for machine '%s'"), def->os.machine); + return -1; + } + if (!cap) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("64-bit PCI hole size setting is not supported " + "with this QEMU binary")); + return -1; + } + + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "%s.pci-hole64-size=%luK", hoststr, + cont->opts.pciopts.pcihole64size); + } + } + + return 0; +} + + +static int qemuBuildCpuModelArgStr(virQEMUDriverPtr driver, const virDomainDef *def, virBufferPtr buf, @@ -7566,52 +7624,8 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildBootCommandLine(cmd, def, qemuCaps, &emitBootindex) < 0) goto error; - for (i = 0; i < def->ncontrollers; i++) { - virDomainControllerDefPtr cont = def->controllers[i]; - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - cont->opts.pciopts.pcihole64) { - const char *hoststr = NULL; - bool cap = false; - bool machine = false; - - switch (cont->model) { - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - hoststr = "i440FX-pcihost"; - cap = virQEMUCapsGet(qemuCaps, QEMU_CAPS_I440FX_PCI_HOLE64_SIZE); - machine = qemuDomainMachineIsI440FX(def); - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: - hoststr = "q35-pcihost"; - cap = virQEMUCapsGet(qemuCaps, QEMU_CAPS_Q35_PCI_HOLE64_SIZE); - machine = qemuDomainMachineIsQ35(def); - break; - - default: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("64-bit PCI hole setting is only for root" - " PCI controllers")); - goto error; - } - - if (!machine) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Setting the 64-bit PCI hole size is not " - "supported for machine '%s'"), def->os.machine); - goto error; - } - if (!cap) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("64-bit PCI hole size setting is not supported " - "with this QEMU binary")); - goto error; - } - - virCommandAddArg(cmd, "-global"); - virCommandAddArgFormat(cmd, "%s.pci-hole64-size=%luK", hoststr, - cont->opts.pciopts.pcihole64size); - } - } + if (qemuBuildGlobalControllerCommandLine(cmd, def, qemuCaps) < 0) + goto error; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { for (j = 0; j < ARRAY_CARDINALITY(contOrder); j++) { -- 2.5.0

On 03/08/2016 02:03 PM, John Ferlan wrote:
Add new function to manage adding the -global controller options to the command line removing that task from the mainline qemuBuildCommandLine.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 106 +++++++++++++++++++++++++++--------------------- 1 file changed, 60 insertions(+), 46 deletions(-)
ACK - Cole

Add new function to manage adding the controller -device options to the command line removing that task from the mainline qemuBuildCommandLine. Also adjust to using const virDomainDef instead of virDomainDefPtr. This causes collateral damage in order to modify called APIs to use the const virDomainDef instead as well. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 10 +- src/conf/domain_conf.h | 4 +- src/qemu/qemu_command.c | 224 ++++++++++++++++++++++------------------- src/qemu/qemu_command.h | 4 +- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_domain_address.h | 2 +- 6 files changed, 132 insertions(+), 114 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d376a2c..11ac707 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13335,8 +13335,9 @@ void virDomainControllerInsertPreAlloced(virDomainDefPtr def, } int -virDomainControllerFind(virDomainDefPtr def, - int type, int idx) +virDomainControllerFind(const virDomainDef *def, + int type, + int idx) { size_t i; @@ -13364,8 +13365,9 @@ virDomainControllerFindUnusedIndex(virDomainDefPtr def, int type) const char * -virDomainControllerAliasFind(virDomainDefPtr def, - int type, int idx) +virDomainControllerAliasFind(const virDomainDef *def, + int type, + int idx) { int contIndex; const char *contTypeStr = virDomainControllerTypeToString(type); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cb7b0e2..8af3c64 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2829,12 +2829,12 @@ int virDomainControllerInsert(virDomainDefPtr def, ATTRIBUTE_RETURN_CHECK; void virDomainControllerInsertPreAlloced(virDomainDefPtr def, virDomainControllerDefPtr controller); -int virDomainControllerFind(virDomainDefPtr def, int type, int idx); +int virDomainControllerFind(const virDomainDef *def, int type, int idx); int virDomainControllerFindByType(virDomainDefPtr def, int type); int virDomainControllerFindByPCIAddress(virDomainDefPtr def, virDevicePCIAddressPtr addr); virDomainControllerDefPtr virDomainControllerRemove(virDomainDefPtr def, size_t i); -const char *virDomainControllerAliasFind(virDomainDefPtr def, +const char *virDomainControllerAliasFind(const virDomainDef *def, int type, int idx) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 87d22f9..bf2cb1a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -276,7 +276,7 @@ char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk, static int qemuBuildDeviceAddressStr(virBufferPtr buf, - virDomainDefPtr domainDef, + const virDomainDef *domainDef, virDomainDeviceInfoPtr info, virQEMUCapsPtr qemuCaps) { @@ -925,7 +925,7 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) * an error and return false; otherwise, return true. */ bool -qemuCheckCCWS390AddressSupport(virDomainDefPtr def, +qemuCheckCCWS390AddressSupport(const virDomainDef *def, virDomainDeviceInfo info, virQEMUCapsPtr qemuCaps, const char *devicename) @@ -1904,7 +1904,7 @@ qemuControllerModelUSBToCaps(int model) static int -qemuBuildUSBControllerDevStr(virDomainDefPtr domainDef, +qemuBuildUSBControllerDevStr(const virDomainDef *domainDef, virDomainControllerDefPtr def, virQEMUCapsPtr qemuCaps, virBuffer *buf) @@ -1942,7 +1942,7 @@ qemuBuildUSBControllerDevStr(virDomainDefPtr domainDef, } char * -qemuBuildControllerDevStr(virDomainDefPtr domainDef, +qemuBuildControllerDevStr(const virDomainDef *domainDef, virDomainControllerDefPtr def, virQEMUCapsPtr qemuCaps, int *nusbcontroller) @@ -2300,6 +2300,120 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, } +static int +qemuBuildControllerDevCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + size_t i, j; + int usbcontroller = 0; + bool usblegacy = false; + int contOrder[] = { + /* + * List of controller types that we add commandline args for, + * *in the order we want to add them*. + * + * The floppy controller is implicit on PIIX4 and older Q35 + * machines. For newer Q35 machines it is added out of the + * controllers loop, after the floppy drives. + * + * We don't add PCI/PCIe root controller either, because it's + * implicit, but we do add PCI bridges and other PCI + * controllers, so we leave that in to check each + * one. Likewise, we don't do anything for the primary IDE + * controller on an i440fx machine or primary SATA on q35, but + * we do add those beyond these two exceptions. + */ + VIR_DOMAIN_CONTROLLER_TYPE_PCI, + VIR_DOMAIN_CONTROLLER_TYPE_USB, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI, + VIR_DOMAIN_CONTROLLER_TYPE_IDE, + VIR_DOMAIN_CONTROLLER_TYPE_SATA, + VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, + VIR_DOMAIN_CONTROLLER_TYPE_CCID, + }; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) + goto check_add_usb; + + for (j = 0; j < ARRAY_CARDINALITY(contOrder); j++) { + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr cont = def->controllers[i]; + char *devstr; + + if (cont->type != contOrder[j]) + continue; + + /* skip USB controllers with type none.*/ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { + usbcontroller = -1; /* mark we don't want a controller */ + continue; + } + + /* skip pci-root/pcie-root */ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) + continue; + + /* first SATA controller on Q35 machines is implicit */ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && + cont->idx == 0 && qemuDomainMachineIsQ35(def)) + continue; + + /* first IDE controller is implicit on various machines */ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && + cont->idx == 0 && qemuDomainMachineHasBuiltinIDE(def)) + continue; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + cont->model == -1 && + !qemuDomainMachineIsQ35(def)) { + bool need_legacy = false; + + /* We're not using legacy usb controller for q35 */ + if (ARCH_IS_PPC64(def->os.arch)) { + /* For ppc64 the legacy was OHCI */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) + need_legacy = true; + } else { + /* For anything else, we used PIIX3_USB_UHCI */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) + need_legacy = true; + } + + if (need_legacy) { + if (usblegacy) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiple legacy USB controllers are " + "not supported")); + return -1; + } + usblegacy = true; + continue; + } + } + + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps, + &usbcontroller))) + return -1; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); + } + } + + check_add_usb: + if (usbcontroller == 0 && + !qemuDomainMachineIsQ35(def) && + !ARCH_IS_S390(def->os.arch)) + virCommandAddArg(cmd, "-usb"); + + return 0; +} + + /** * qemuBuildMemoryBackendStr: * @size: size of the memory device in kibibytes @@ -7485,33 +7599,7 @@ qemuBuildCommandLine(virConnectPtr conn, int last_good_net = -1; virCommandPtr cmd = NULL; bool emitBootindex = false; - int usbcontroller = 0; int actualSerials = 0; - bool usblegacy = false; - int contOrder[] = { - /* - * List of controller types that we add commandline args for, - * *in the order we want to add them*. - * - * The floppy controller is implicit on PIIX4 and older Q35 - * machines. For newer Q35 machines it is added out of the - * controllers loop, after the floppy drives. - * - * We don't add PCI/PCIe root controller either, because it's - * implicit, but we do add PCI bridges and other PCI - * controllers, so we leave that in to check each - * one. Likewise, we don't do anything for the primary IDE - * controller on an i440fx machine or primary SATA on q35, but - * we do add those beyond these two exceptions. - */ - VIR_DOMAIN_CONTROLLER_TYPE_PCI, - VIR_DOMAIN_CONTROLLER_TYPE_USB, - VIR_DOMAIN_CONTROLLER_TYPE_SCSI, - VIR_DOMAIN_CONTROLLER_TYPE_IDE, - VIR_DOMAIN_CONTROLLER_TYPE_SATA, - VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, - VIR_DOMAIN_CONTROLLER_TYPE_CCID, - }; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virBuffer fdc_opts = VIR_BUFFER_INITIALIZER; char *fdc_opts_str = NULL; @@ -7627,80 +7715,8 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildGlobalControllerCommandLine(cmd, def, qemuCaps) < 0) goto error; - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - for (j = 0; j < ARRAY_CARDINALITY(contOrder); j++) { - for (i = 0; i < def->ncontrollers; i++) { - virDomainControllerDefPtr cont = def->controllers[i]; - char *devstr; - - if (cont->type != contOrder[j]) - continue; - - /* skip USB controllers with type none.*/ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { - usbcontroller = -1; /* mark we don't want a controller */ - continue; - } - - /* skip pci-root/pcie-root */ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) - continue; - - /* first SATA controller on Q35 machines is implicit */ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && - cont->idx == 0 && qemuDomainMachineIsQ35(def)) - continue; - - /* first IDE controller is implicit on various machines */ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && - cont->idx == 0 && qemuDomainMachineHasBuiltinIDE(def)) - continue; - - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && - cont->model == -1 && - !qemuDomainMachineIsQ35(def)) { - bool need_legacy = false; - - /* We're not using legacy usb controller for q35 */ - if (ARCH_IS_PPC64(def->os.arch)) { - /* For ppc64 the legacy was OHCI */ - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) - need_legacy = true; - } else { - /* For anything else, we used PIIX3_USB_UHCI */ - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) - need_legacy = true; - } - - if (need_legacy) { - if (usblegacy) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Multiple legacy USB controllers are " - "not supported")); - goto error; - } - usblegacy = true; - continue; - } - } - - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps, - &usbcontroller))) - goto error; - virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); - } - } - } - - if (usbcontroller == 0 && - !qemuDomainMachineIsQ35(def) && - !ARCH_IS_S390(def->os.arch)) - virCommandAddArg(cmd, "-usb"); + if (qemuBuildControllerDevCommandLine(cmd, def, qemuCaps) < 0) + goto error; for (i = 0; i < def->nhubs; i++) { virDomainHubDefPtr hub = def->hubs[i]; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index c63c7bc..ec18869 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -127,7 +127,7 @@ char *qemuBuildFSDevStr(virDomainDefPtr domainDef, virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps); /* Current, best practice */ -char *qemuBuildControllerDevStr(virDomainDefPtr domainDef, +char *qemuBuildControllerDevStr(const virDomainDef *domainDef, virDomainControllerDefPtr def, virQEMUCapsPtr qemuCaps, int *nusbcontroller); @@ -223,7 +223,7 @@ int qemuCheckDiskConfig(virDomainDiskDefPtr disk); bool qemuCheckFips(void); -bool qemuCheckCCWS390AddressSupport(virDomainDefPtr def, +bool qemuCheckCCWS390AddressSupport(const virDomainDef *def, virDomainDeviceInfo info, virQEMUCapsPtr qemuCaps, const char *devicename); diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index eff33fc..4f31484 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -40,7 +40,7 @@ VIR_LOG_INIT("qemu.qemu_domain_address"); int -qemuDomainSetSCSIControllerModel(virDomainDefPtr def, +qemuDomainSetSCSIControllerModel(const virDomainDef *def, virQEMUCapsPtr qemuCaps, int *model) { diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index 855f4e5..50019b8 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -27,7 +27,7 @@ # include "domain_conf.h" # include "qemu_capabilities.h" -int qemuDomainSetSCSIControllerModel(virDomainDefPtr def, +int qemuDomainSetSCSIControllerModel(const virDomainDef *def, virQEMUCapsPtr qemuCaps, int *model); -- 2.5.0

On 03/08/2016 02:03 PM, John Ferlan wrote:
Add new function to manage adding the controller -device options to the command line removing that task from the mainline qemuBuildCommandLine.
Also adjust to using const virDomainDef instead of virDomainDefPtr. This causes collateral damage in order to modify called APIs to use the const virDomainDef instead as well.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 10 +- src/conf/domain_conf.h | 4 +- src/qemu/qemu_command.c | 224 ++++++++++++++++++++++------------------- src/qemu/qemu_command.h | 4 +- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_domain_address.h | 2 +- 6 files changed, 132 insertions(+), 114 deletions(-)
The check_add_usb: bit isn't really needed since the !QEMU_CAPS_DEVICE bit will never be triggered nowadays... but that's a larger cleanup that it isn't worth mixing up with these reworks ACK - Cole

Add new function to manage adding the hub -device options to the command line removing that task from the mainline qemuBuildCommandLine. Also make qemuBuildHubDevStr static to the module since it's only used here. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 38 ++++++++++++++++++++++++++------------ src/qemu/qemu_command.h | 3 --- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bf2cb1a..ef94af8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3744,8 +3744,8 @@ qemuBuildUSBHostdevDevStr(virDomainDefPtr def, } -char * -qemuBuildHubDevStr(virDomainDefPtr def, +static char * +qemuBuildHubDevStr(const virDomainDef *def, virDomainHubDefPtr dev, virQEMUCapsPtr qemuCaps) { @@ -3780,6 +3780,28 @@ qemuBuildHubDevStr(virDomainDefPtr def, } +static int +qemuBuildHubCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + size_t i; + + for (i = 0; i < def->nhubs; i++) { + virDomainHubDefPtr hub = def->hubs[i]; + char *optstr; + + virCommandAddArg(cmd, "-device"); + if (!(optstr = qemuBuildHubDevStr(def, hub, qemuCaps))) + return -1; + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); + } + + return 0; +} + + char * qemuBuildUSBHostdevUSBDevStr(virDomainHostdevDefPtr dev) { @@ -7718,16 +7740,8 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildControllerDevCommandLine(cmd, def, qemuCaps) < 0) goto error; - for (i = 0; i < def->nhubs; i++) { - virDomainHubDefPtr hub = def->hubs[i]; - char *optstr; - - virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildHubDevStr(def, hub, qemuCaps))) - goto error; - virCommandAddArg(cmd, optstr); - VIR_FREE(optstr); - } + if (qemuBuildHubCommandLine(cmd, def, qemuCaps) < 0) + goto error; if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || emitBootindex)) { /* bootDevs will get translated into either bootindex=N or boot=on diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index ec18869..ed4f5bd 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -205,9 +205,6 @@ char *qemuBuildSCSIHostdevDevStr(virDomainDefPtr def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); -char *qemuBuildHubDevStr(virDomainDefPtr def, - virDomainHubDefPtr dev, - virQEMUCapsPtr qemuCaps); char *qemuBuildRedirdevDevStr(virDomainDefPtr def, virDomainRedirdevDefPtr dev, virQEMUCapsPtr qemuCaps); -- 2.5.0

On 03/08/2016 02:03 PM, John Ferlan wrote:
Add new function to manage adding the hub -device options to the command line removing that task from the mainline qemuBuildCommandLine.
Also make qemuBuildHubDevStr static to the module since it's only used here.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 38 ++++++++++++++++++++++++++------------ src/qemu/qemu_command.h | 3 --- 2 files changed, 26 insertions(+), 15 deletions(-)
ACK - Cole

Add new function to manage adding the disk -drive options to the command line removing that task from the mainline qemuBuildCommandLine. Also since using const virDomainDef in new function, that means other functions called needed to change their usage. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 4 +- src/conf/domain_conf.h | 4 +- src/qemu/qemu_command.c | 317 +++++++++++++++++++++++++----------------------- src/qemu/qemu_command.h | 2 +- 4 files changed, 173 insertions(+), 154 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 11ac707..4b2633a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5746,7 +5746,7 @@ virDomainHostdevDefParseXMLCaps(xmlNodePtr node ATTRIBUTE_UNUSED, } int -virDomainDeviceFindControllerModel(virDomainDefPtr def, +virDomainDeviceFindControllerModel(const virDomainDef *def, virDomainDeviceInfoPtr info, int controllerType) { @@ -18327,7 +18327,7 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def) } virDomainIOThreadIDDefPtr -virDomainIOThreadIDFind(virDomainDefPtr def, +virDomainIOThreadIDFind(const virDomainDef *def, unsigned int iothread_id) { size_t i; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8af3c64..468b759 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2516,7 +2516,7 @@ int virDomainDiskSetDriver(virDomainDiskDefPtr def, const char *name) ATTRIBUTE_RETURN_CHECK; int virDomainDiskGetFormat(virDomainDiskDefPtr def); void virDomainDiskSetFormat(virDomainDiskDefPtr def, int format); -int virDomainDeviceFindControllerModel(virDomainDefPtr def, +int virDomainDeviceFindControllerModel(const virDomainDef *def, virDomainDeviceInfoPtr info, int controllerType); virDomainDiskDefPtr virDomainDiskFindByBusAndDst(virDomainDefPtr def, @@ -2699,7 +2699,7 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src, int virDomainDefAddImplicitControllers(virDomainDefPtr def); -virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(virDomainDefPtr def, +virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(const virDomainDef *def, unsigned int iothread_id); virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int iothread_id); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ef94af8..372f84f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1443,7 +1443,7 @@ qemuBuildDriveStr(virConnectPtr conn, static bool -qemuCheckIOThreads(virDomainDefPtr def, +qemuCheckIOThreads(const virDomainDef *def, virDomainDiskDefPtr disk) { /* Right "type" of disk" */ @@ -1469,7 +1469,7 @@ qemuCheckIOThreads(virDomainDefPtr def, char * -qemuBuildDriveDevStr(virDomainDefPtr def, +qemuBuildDriveDevStr(const virDomainDef *def, virDomainDiskDefPtr disk, int bootindex, virQEMUCapsPtr qemuCaps) @@ -1766,6 +1766,168 @@ qemuBuildDriveDevStr(virDomainDefPtr def, } +static int +qemuBuildDiskDriveCommandLine(virCommandPtr cmd, + virConnectPtr conn, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps, + bool emitBootindex) +{ + size_t i; + int bootCD = 0, bootFloppy = 0, bootDisk = 0; + virBuffer fdc_opts = VIR_BUFFER_INITIALIZER; + char *fdc_opts_str = NULL; + + if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || emitBootindex)) { + /* bootDevs will get translated into either bootindex=N or boot=on + * depending on what qemu supports */ + for (i = 0; i < def->os.nBootDevs; i++) { + switch (def->os.bootDevs[i]) { + case VIR_DOMAIN_BOOT_CDROM: + bootCD = i + 1; + break; + case VIR_DOMAIN_BOOT_FLOPPY: + bootFloppy = i + 1; + break; + case VIR_DOMAIN_BOOT_DISK: + bootDisk = i + 1; + break; + } + } + } + + for (i = 0; i < def->ndisks; i++) { + char *optstr; + int bootindex = 0; + virDomainDiskDefPtr disk = def->disks[i]; + bool withDeviceArg = false; + bool deviceFlagMasked = false; + + /* Unless we have -device, then USB disks need special + handling */ + if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + virCommandAddArg(cmd, "-usbdevice"); + virCommandAddArgFormat(cmd, "disk:%s", disk->src->path); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported usb disk type for '%s'"), + disk->src->path); + return -1; + } + continue; + } + + /* PowerPC pseries based VMs do not support floppy device */ + if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) && + ARCH_IS_PPC64(def->os.arch) && + STRPREFIX(def->os.machine, "pseries")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PowerPC pseries machines do not support floppy device")); + return -1; + } + + switch (disk->device) { + case VIR_DOMAIN_DISK_DEVICE_CDROM: + bootindex = bootCD; + bootCD = 0; + break; + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + bootindex = bootFloppy; + bootFloppy = 0; + break; + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: + bootindex = bootDisk; + bootDisk = 0; + break; + } + + virCommandAddArg(cmd, "-drive"); + + /* Unfortunately it is not possible to use + -device for floppies, xen PV, or SD + devices. Fortunately, those don't need + static PCI addresses, so we don't really + care that we can't use -device */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + if (disk->bus != VIR_DOMAIN_DISK_BUS_XEN && + disk->bus != VIR_DOMAIN_DISK_BUS_SD) { + withDeviceArg = true; + } else { + virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE); + deviceFlagMasked = true; + } + } + optstr = qemuBuildDriveStr(conn, disk, + emitBootindex ? false : !!bootindex, + qemuCaps); + if (deviceFlagMasked) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE); + if (!optstr) + return -1; + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); + + if (!emitBootindex) + bootindex = 0; + else if (disk->info.bootIndex) + bootindex = disk->info.bootIndex; + + if (withDeviceArg) { + if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { + if (virAsprintf(&optstr, "drive%c=drive-%s", + disk->info.addr.drive.unit ? 'B' : 'A', + disk->info.alias) < 0) + return -1; + + if (!qemuDomainMachineNeedsFDC(def)) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "isa-fdc.%s", optstr); + } else { + virBufferAsprintf(&fdc_opts, "%s,", optstr); + } + VIR_FREE(optstr); + + if (bootindex) { + if (virAsprintf(&optstr, "bootindex%c=%d", + disk->info.addr.drive.unit + ? 'B' : 'A', + bootindex) < 0) + return -1; + + if (!qemuDomainMachineNeedsFDC(def)) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "isa-fdc.%s", optstr); + } else { + virBufferAsprintf(&fdc_opts, "%s,", optstr); + } + VIR_FREE(optstr); + } + } else { + virCommandAddArg(cmd, "-device"); + + if (!(optstr = qemuBuildDriveDevStr(def, disk, bootindex, + qemuCaps))) + return -1; + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); + } + } + } + /* Newer Q35 machine types require an explicit FDC controller */ + virBufferTrim(&fdc_opts, ",", -1); + if ((fdc_opts_str = virBufferContentAndReset(&fdc_opts))) { + virCommandAddArg(cmd, "-device"); + virCommandAddArgFormat(cmd, "isa-fdc,%s", fdc_opts_str); + VIR_FREE(fdc_opts_str); + } + + return 0; +} + + char *qemuBuildFSStr(virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED) { @@ -7623,9 +7785,7 @@ qemuBuildCommandLine(virConnectPtr conn, bool emitBootindex = false; int actualSerials = 0; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - virBuffer fdc_opts = VIR_BUFFER_INITIALIZER; - char *fdc_opts_str = NULL; - int bootCD = 0, bootFloppy = 0, bootDisk = 0, bootHostdevNet = 0; + int bootHostdevNet = 0; VIR_DEBUG("conn=%p driver=%p def=%p mon=%p json=%d " @@ -7743,150 +7903,9 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildHubCommandLine(cmd, def, qemuCaps) < 0) goto error; - if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || emitBootindex)) { - /* bootDevs will get translated into either bootindex=N or boot=on - * depending on what qemu supports */ - for (i = 0; i < def->os.nBootDevs; i++) { - switch (def->os.bootDevs[i]) { - case VIR_DOMAIN_BOOT_CDROM: - bootCD = i + 1; - break; - case VIR_DOMAIN_BOOT_FLOPPY: - bootFloppy = i + 1; - break; - case VIR_DOMAIN_BOOT_DISK: - bootDisk = i + 1; - break; - } - } - } - - for (i = 0; i < def->ndisks; i++) { - char *optstr; - int bootindex = 0; - virDomainDiskDefPtr disk = def->disks[i]; - bool withDeviceArg = false; - bool deviceFlagMasked = false; - - /* Unless we have -device, then USB disks need special - handling */ - if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - virCommandAddArg(cmd, "-usbdevice"); - virCommandAddArgFormat(cmd, "disk:%s", disk->src->path); - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported usb disk type for '%s'"), - disk->src->path); - goto error; - } - continue; - } - - /* PowerPC pseries based VMs do not support floppy device */ - if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) && - ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("PowerPC pseries machines do not support floppy device")); - goto error; - } - - switch (disk->device) { - case VIR_DOMAIN_DISK_DEVICE_CDROM: - bootindex = bootCD; - bootCD = 0; - break; - case VIR_DOMAIN_DISK_DEVICE_FLOPPY: - bootindex = bootFloppy; - bootFloppy = 0; - break; - case VIR_DOMAIN_DISK_DEVICE_DISK: - case VIR_DOMAIN_DISK_DEVICE_LUN: - bootindex = bootDisk; - bootDisk = 0; - break; - } - - virCommandAddArg(cmd, "-drive"); - - /* Unfortunately it is not possible to use - -device for floppies, xen PV, or SD - devices. Fortunately, those don't need - static PCI addresses, so we don't really - care that we can't use -device */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (disk->bus != VIR_DOMAIN_DISK_BUS_XEN && - disk->bus != VIR_DOMAIN_DISK_BUS_SD) { - withDeviceArg = true; - } else { - virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE); - deviceFlagMasked = true; - } - } - optstr = qemuBuildDriveStr(conn, disk, - emitBootindex ? false : !!bootindex, - qemuCaps); - if (deviceFlagMasked) - virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE); - if (!optstr) - goto error; - virCommandAddArg(cmd, optstr); - VIR_FREE(optstr); - - if (!emitBootindex) - bootindex = 0; - else if (disk->info.bootIndex) - bootindex = disk->info.bootIndex; - - if (withDeviceArg) { - if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { - if (virAsprintf(&optstr, "drive%c=drive-%s", - disk->info.addr.drive.unit ? 'B' : 'A', - disk->info.alias) < 0) - goto error; - - if (!qemuDomainMachineNeedsFDC(def)) { - virCommandAddArg(cmd, "-global"); - virCommandAddArgFormat(cmd, "isa-fdc.%s", optstr); - } else { - virBufferAsprintf(&fdc_opts, "%s,", optstr); - } - VIR_FREE(optstr); - - if (bootindex) { - if (virAsprintf(&optstr, "bootindex%c=%d", - disk->info.addr.drive.unit - ? 'B' : 'A', - bootindex) < 0) - goto error; - - if (!qemuDomainMachineNeedsFDC(def)) { - virCommandAddArg(cmd, "-global"); - virCommandAddArgFormat(cmd, "isa-fdc.%s", optstr); - } else { - virBufferAsprintf(&fdc_opts, "%s,", optstr); - } - VIR_FREE(optstr); - } - } else { - virCommandAddArg(cmd, "-device"); - - if (!(optstr = qemuBuildDriveDevStr(def, disk, bootindex, - qemuCaps))) - goto error; - virCommandAddArg(cmd, optstr); - VIR_FREE(optstr); - } - } - } - /* Newer Q35 machine types require an explicit FDC controller */ - virBufferTrim(&fdc_opts, ",", -1); - if ((fdc_opts_str = virBufferContentAndReset(&fdc_opts))) { - virCommandAddArg(cmd, "-device"); - virCommandAddArgFormat(cmd, "isa-fdc,%s", fdc_opts_str); - VIR_FREE(fdc_opts_str); - } + if (qemuBuildDiskDriveCommandLine(cmd, conn, def, qemuCaps, + emitBootindex) < 0) + goto error; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) { for (i = 0; i < def->nfss; i++) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index ed4f5bd..b10fc29 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -119,7 +119,7 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps); /* Current, best practice */ -char *qemuBuildDriveDevStr(virDomainDefPtr def, +char *qemuBuildDriveDevStr(const virDomainDef *def, virDomainDiskDefPtr disk, int bootindex, virQEMUCapsPtr qemuCaps); -- 2.5.0

On 03/08/2016 02:03 PM, John Ferlan wrote:
Add new function to manage adding the disk -drive options to the command line removing that task from the mainline qemuBuildCommandLine.
Also since using const virDomainDef in new function, that means other functions called needed to change their usage.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 4 +- src/conf/domain_conf.h | 4 +- src/qemu/qemu_command.c | 317 +++++++++++++++++++++++++----------------------- src/qemu/qemu_command.h | 2 +- 4 files changed, 173 insertions(+), 154 deletions(-)
ACK - Cole

Add new function to manage adding the -fsdev options to the command line removing that task from the mainline qemuBuildCommandLine. Since both qemuBuildFSStr and qemuBuildFSDevStr are local, make them static and fix their prototypes to use the const virDomainDef as well. Make some minor formatting changes for long lines. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 74 ++++++++++++++++++++++++++++++------------------- src/qemu/qemu_command.h | 6 +--- 2 files changed, 46 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 372f84f..caf6f37 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1928,8 +1928,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, } -char *qemuBuildFSStr(virDomainFSDefPtr fs, - virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED) +static char * +qemuBuildFSStr(virDomainFSDefPtr fs, + virQEMUCapsPtr qemuCaps) { virBuffer opt = VIR_BUFFER_INITIALIZER; const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver); @@ -2002,8 +2003,8 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, } -char * -qemuBuildFSDevStr(virDomainDefPtr def, +static char * +qemuBuildFSDevStr(const virDomainDef *def, virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps) { @@ -2021,7 +2022,8 @@ qemuBuildFSDevStr(virDomainDefPtr def, virBufferAddLit(&opt, "virtio-9p-pci"); virBufferAsprintf(&opt, ",id=%s", fs->info.alias); - virBufferAsprintf(&opt, ",fsdev=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); + virBufferAsprintf(&opt, ",fsdev=%s%s", + QEMU_FSDEV_HOST_PREFIX, fs->info.alias); virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) @@ -2039,6 +2041,42 @@ qemuBuildFSDevStr(virDomainDefPtr def, static int +qemuBuildFSDevCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + size_t i; + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) { + for (i = 0; i < def->nfss; i++) { + char *optstr; + virDomainFSDefPtr fs = def->fss[i]; + + virCommandAddArg(cmd, "-fsdev"); + if (!(optstr = qemuBuildFSStr(fs, qemuCaps))) + return -1; + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); + + virCommandAddArg(cmd, "-device"); + if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) + return -1; + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); + } + } else { + if (def->nfss) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("filesystem passthrough not supported by this QEMU")); + return -1; + } + } + + return 0; +} + + +static int qemuControllerModelUSBToCaps(int model) { switch (model) { @@ -7907,30 +7945,8 @@ qemuBuildCommandLine(virConnectPtr conn, emitBootindex) < 0) goto error; - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) { - for (i = 0; i < def->nfss; i++) { - char *optstr; - virDomainFSDefPtr fs = def->fss[i]; - - virCommandAddArg(cmd, "-fsdev"); - if (!(optstr = qemuBuildFSStr(fs, qemuCaps))) - goto error; - virCommandAddArg(cmd, optstr); - VIR_FREE(optstr); - - virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) - goto error; - virCommandAddArg(cmd, optstr); - VIR_FREE(optstr); - } - } else { - if (def->nfss) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("filesystem passthrough not supported by this QEMU")); - goto error; - } - } + if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0) + goto error; if (!def->nnets) { /* If we have -device, then we set -nodefault already */ diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index b10fc29..6e22872 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -115,17 +115,13 @@ char *qemuBuildDriveStr(virConnectPtr conn, virDomainDiskDefPtr disk, bool bootable, virQEMUCapsPtr qemuCaps); -char *qemuBuildFSStr(virDomainFSDefPtr fs, - virQEMUCapsPtr qemuCaps); /* Current, best practice */ char *qemuBuildDriveDevStr(const virDomainDef *def, virDomainDiskDefPtr disk, int bootindex, virQEMUCapsPtr qemuCaps); -char *qemuBuildFSDevStr(virDomainDefPtr domainDef, - virDomainFSDefPtr fs, - virQEMUCapsPtr qemuCaps); + /* Current, best practice */ char *qemuBuildControllerDevStr(const virDomainDef *domainDef, virDomainControllerDefPtr def, -- 2.5.0

On 03/08/2016 02:03 PM, John Ferlan wrote:
Add new function to manage adding the -fsdev options to the command line removing that task from the mainline qemuBuildCommandLine.
Since both qemuBuildFSStr and qemuBuildFSDevStr are local, make them static and fix their prototypes to use the const virDomainDef as well. Make some minor formatting changes for long lines.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 74 ++++++++++++++++++++++++++++++------------------- src/qemu/qemu_command.h | 6 +--- 2 files changed, 46 insertions(+), 34 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 372f84f..caf6f37 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1928,8 +1928,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, }
-char *qemuBuildFSStr(virDomainFSDefPtr fs, - virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED) +static char * +qemuBuildFSStr(virDomainFSDefPtr fs, + virQEMUCapsPtr qemuCaps) { virBuffer opt = VIR_BUFFER_INITIALIZER; const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver); @@ -2002,8 +2003,8 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, }
-char * -qemuBuildFSDevStr(virDomainDefPtr def, +static char * +qemuBuildFSDevStr(const virDomainDef *def, virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps) { @@ -2021,7 +2022,8 @@ qemuBuildFSDevStr(virDomainDefPtr def, virBufferAddLit(&opt, "virtio-9p-pci");
virBufferAsprintf(&opt, ",id=%s", fs->info.alias); - virBufferAsprintf(&opt, ",fsdev=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); + virBufferAsprintf(&opt, ",fsdev=%s%s", + QEMU_FSDEV_HOST_PREFIX, fs->info.alias); virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst);
if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) @@ -2039,6 +2041,42 @@ qemuBuildFSDevStr(virDomainDefPtr def,
static int +qemuBuildFSDevCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + size_t i; + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) { + for (i = 0; i < def->nfss; i++) { + char *optstr; + virDomainFSDefPtr fs = def->fss[i]; + + virCommandAddArg(cmd, "-fsdev"); + if (!(optstr = qemuBuildFSStr(fs, qemuCaps))) + return -1; + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); + + virCommandAddArg(cmd, "-device"); + if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) + return -1; + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); + } + } else { + if (def->nfss) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("filesystem passthrough not supported by this QEMU")); + return -1; + } + } +
Can you do the (!QEMU_CAPS_FSDEV && def->nfss) return -1; check up front, so the logic doesn't need to be indented? ACK with that if you agree - Cole

Add new function to manage adding the network device options to the command line removing that task from the mainline qemuBuildCommandLine. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 133 +++++++++++++++++++++++++++++------------------- 1 file changed, 82 insertions(+), 51 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index caf6f37..20c0147 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7274,6 +7274,84 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, return ret; } + +/* NOTE: Not using const virDomainDef here since eventually a call is made + * into virSecurityManagerSetTapFDLabel which calls it's driver + * API domainSetSecurityTapFDLabel that doesn't use the const format. + */ +static int +qemuBuildNetCommandLine(virCommandPtr cmd, + virQEMUDriverPtr driver, + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virNetDevVPortProfileOp vmop, + bool standalone, + bool emitBootindex, + size_t *nnicindexes, + int **nicindexes, + int *bootHostdevNet) +{ + size_t i; + int last_good_net = -1; + + if (!def->nnets) { + /* If we have -device, then we set -nodefault already */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) + virCommandAddArgList(cmd, "-net", "none", NULL); + } else { + int bootNet = 0; + + if (emitBootindex) { + /* convert <boot dev='network'/> to bootindex since we didn't emit + * -boot n + */ + for (i = 0; i < def->os.nBootDevs; i++) { + if (def->os.bootDevs[i] == VIR_DOMAIN_BOOT_NET) { + bootNet = i + 1; + break; + } + } + } + + for (i = 0; i < def->nnets; i++) { + virDomainNetDefPtr net = def->nets[i]; + int vlan; + + /* VLANs are not used with -netdev, so don't record them */ + if (qemuDomainSupportsNetdev(def, qemuCaps, net)) + vlan = -1; + else + vlan = i; + + if (qemuBuildInterfaceCommandLine(cmd, driver, def, net, + qemuCaps, vlan, bootNet, vmop, + standalone, nnicindexes, + nicindexes) < 0) + goto error; + + last_good_net = i; + /* if this interface is a type='hostdev' interface and we + * haven't yet added a "bootindex" parameter to an + * emulated network device, save the bootindex - hostdev + * interface commandlines will be built later on when we + * cycle through all the hostdevs, and we'll use it then. + */ + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV && + *bootHostdevNet == 0) { + *bootHostdevNet = bootNet; + } + bootNet = 0; + } + } + return 0; + + error: + for (i = 0; last_good_net != -1 && i <= last_good_net; i++) + virDomainConfNWFilterTeardown(def->nets[i]); + return -1; +} + + char * qemuBuildShmemDevStr(virDomainDefPtr def, virDomainShmemDefPtr shmem, @@ -7818,7 +7896,6 @@ qemuBuildCommandLine(virConnectPtr conn, size_t i, j; char uuid[VIR_UUID_STRING_BUFLEN]; bool havespice = false; - int last_good_net = -1; virCommandPtr cmd = NULL; bool emitBootindex = false; int actualSerials = 0; @@ -7948,54 +8025,10 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0) goto error; - if (!def->nnets) { - /* If we have -device, then we set -nodefault already */ - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) - virCommandAddArgList(cmd, "-net", "none", NULL); - } else { - int bootNet = 0; - - if (emitBootindex) { - /* convert <boot dev='network'/> to bootindex since we didn't emit - * -boot n - */ - for (i = 0; i < def->os.nBootDevs; i++) { - if (def->os.bootDevs[i] == VIR_DOMAIN_BOOT_NET) { - bootNet = i + 1; - break; - } - } - } - - for (i = 0; i < def->nnets; i++) { - virDomainNetDefPtr net = def->nets[i]; - int vlan; - - /* VLANs are not used with -netdev, so don't record them */ - if (qemuDomainSupportsNetdev(def, qemuCaps, net)) - vlan = -1; - else - vlan = i; - - if (qemuBuildInterfaceCommandLine(cmd, driver, def, net, - qemuCaps, vlan, bootNet, vmop, - standalone, nnicindexes, nicindexes) < 0) - goto error; - - last_good_net = i; - /* if this interface is a type='hostdev' interface and we - * haven't yet added a "bootindex" parameter to an - * emulated network device, save the bootindex - hostdev - * interface commandlines will be built later on when we - * cycle through all the hostdevs, and we'll use it then. - */ - if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV && - bootHostdevNet == 0) { - bootHostdevNet = bootNet; - } - bootNet = 0; - } - } + if (qemuBuildNetCommandLine(cmd, driver, def, qemuCaps, vmop, standalone, + emitBootindex, nnicindexes, nicindexes, + &bootHostdevNet) < 0) + goto error; if (def->nsmartcards) { /* -device usb-ccid was already emitted along with other @@ -9029,8 +9062,6 @@ qemuBuildCommandLine(virConnectPtr conn, /* free up any resources in the network driver * but don't overwrite the original error */ originalError = virSaveLastError(); - for (i = 0; last_good_net != -1 && i <= last_good_net; i++) - virDomainConfNWFilterTeardown(def->nets[i]); virSetError(originalError); virFreeError(originalError); virCommandFree(cmd); -- 2.5.0

On 03/08/2016 02:03 PM, John Ferlan wrote:
Add new function to manage adding the network device options to the command line removing that task from the mainline qemuBuildCommandLine.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 133 +++++++++++++++++++++++++++++------------------- 1 file changed, 82 insertions(+), 51 deletions(-)
ACK - Cole

On Tue, Mar 08, 2016 at 02:03:16PM -0500, John Ferlan wrote:
Add new function to manage adding the network device options to the command line removing that task from the mainline qemuBuildCommandLine.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 133 +++++++++++++++++++++++++++++------------------- 1 file changed, 82 insertions(+), 51 deletions(-)
@@ -9029,8 +9062,6 @@ qemuBuildCommandLine(virConnectPtr conn, /* free up any resources in the network driver * but don't overwrite the original error */
This saving of the error was done because of the virDomainConfNWFilterTeardown call. Removing the call made the *Error calls pointless.
originalError = virSaveLastError(); - for (i = 0; last_good_net != -1 && i <= last_good_net; i++) - virDomainConfNWFilterTeardown(def->nets[i]);
Before, this tore down the filters if any part of the command line building failed. Now it's only done when qemuBuildNetCommandLine fails. Fortunately, it seems these calls were redundant because qemuProcessStop also calls virDomainConfVMNWFilterTeardown. Jan

On 03/08/2016 02:03 PM, John Ferlan wrote:
Rather than hope for a review of 25 patches or creating some remote place to place a branch - I'll repost the original:
http://www.redhat.com/archives/libvir-list/2016-February/msg00975.html
in smaller chunks. No differences in these patches other than a rebase to top of tree a short while ago (commit id 'cf091094a'). There were also no conflicts from changes since initial posting.
John Ferlan (9): qemu: Introduce qemuBuildClockCommandLine qemu: Introduce qemuBuildPMCommandLine qemu: Introduce qemuBuildBootCommandLine qemu: Introduce qemuBuildGlobalControllerCommandLine qemu: Introduce qemuBuildControllerDevCommandLine qemu: Introduce qemuBuildHubCommandLine qemu: Introduce qemuBuildDiskDriveCommandLine qemu: Introduce qemuBuildFSDevCommandLine qemu: Introduce qemuBuildNetCommandLine
src/conf/domain_conf.c | 14 +- src/conf/domain_conf.h | 8 +- src/qemu/qemu_command.c | 5488 +++++++++++++++++++++------------------- src/qemu/qemu_command.h | 15 +- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_domain_address.h | 2 +- 6 files changed, 2843 insertions(+), 2686 deletions(-)
Thanks Cole! Made the suggested adjustment in patch 8 and pushed. John
participants (3)
-
Cole Robinson
-
John Ferlan
-
Ján Tomko