[libvirt PATCH 0/2] improve audio device compat handling on migration
Avoid being over-eager in stripping <audio> elements. This still isn't ideal, because if a user happened to provide an audio config that *exactly* matches the libvirt historical default we'll still strip it. Thinking that maybe libvirt should advertize feature flags in te migration cookies. eg if the QEMU driver on the target host reports "explicit-audiodev", then te source host knows it doesn't need to strip it out to migrate to this host. If it doesn't report it, then it knows it must be an older libvirt version pre-dating audiodev. Daniel P. Berrangé (2): conf: add helper for comparing virDomainAudioDef objects qemu: don't strip audio elements with user config present src/conf/domain_conf.c | 144 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 + src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 72 +++++++++++--------- 4 files changed, 188 insertions(+), 32 deletions(-) -- 2.31.1
It is useful to be able to deeply check them for equality. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 144 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 + src/libvirt_private.syms | 1 + 3 files changed, 148 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index da0c64b460..f919d3f7a0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29842,6 +29842,150 @@ virDomainAudioIOCommonIsSet(virDomainAudioIOCommon *common) common->bufferLength; } + +static bool +virDomainAudioIOCommonIsEqual(virDomainAudioIOCommon *this, + virDomainAudioIOCommon *that) +{ + return this->mixingEngine == that->mixingEngine && + this->fixedSettings == that->fixedSettings && + this->frequency == that->frequency && + this->channels == that->channels && + this->voices == that->voices && + this->format == that->format && + this->bufferLength == that->bufferLength; +} + +static bool +virDomainAudioIOALSAIsEqual(virDomainAudioIOALSA *this, + virDomainAudioIOALSA *that) +{ + return STREQ_NULLABLE(this->dev, that->dev); +} + +static bool +virDomainAudioIOCoreAudioIsEqual(virDomainAudioIOCoreAudio *this, + virDomainAudioIOCoreAudio *that) +{ + return this->bufferCount == that->bufferCount; +} + +static bool +virDomainAudioIOJackIsEqual(virDomainAudioIOJack *this, + virDomainAudioIOJack *that) +{ + return STREQ_NULLABLE(this->serverName, that->serverName) && + STREQ_NULLABLE(this->clientName, that->clientName) && + STREQ_NULLABLE(this->connectPorts, that->connectPorts) && + this->exactName == that->exactName; +} + +static bool +virDomainAudioIOOSSIsEqual(virDomainAudioIOOSS *this, + virDomainAudioIOOSS *that) +{ + return STREQ_NULLABLE(this->dev, that->dev) && + this->bufferCount == that->bufferCount && + this->tryPoll == that->tryPoll; +} + +static bool +virDomainAudioIOPulseAudioIsEqual(virDomainAudioIOPulseAudio *this, + virDomainAudioIOPulseAudio *that) +{ + return STREQ_NULLABLE(this->name, that->name) && + STREQ_NULLABLE(this->streamName, that->streamName) && + this->latency == that->latency; +} + +static bool +virDomainAudioIOSDLIsEqual(virDomainAudioIOSDL *this, + virDomainAudioIOSDL *that) +{ + return this->bufferCount == that->bufferCount; +} + + +static bool +virDomainAudioBackendIsEqual(virDomainAudioDef *this, + virDomainAudioDef *that) +{ + if (this->type != that->type) + return false; + + switch (this->type) { + case VIR_DOMAIN_AUDIO_TYPE_NONE: + return true; + + case VIR_DOMAIN_AUDIO_TYPE_ALSA: + return virDomainAudioIOALSAIsEqual(&this->backend.alsa.input, + &that->backend.alsa.input) && + virDomainAudioIOALSAIsEqual(&this->backend.alsa.output, + &that->backend.alsa.output); + + case VIR_DOMAIN_AUDIO_TYPE_COREAUDIO: + return virDomainAudioIOCoreAudioIsEqual(&this->backend.coreaudio.input, + &that->backend.coreaudio.input) && + virDomainAudioIOCoreAudioIsEqual(&this->backend.coreaudio.output, + &that->backend.coreaudio.output); + + case VIR_DOMAIN_AUDIO_TYPE_JACK: + return virDomainAudioIOJackIsEqual(&this->backend.jack.input, + &that->backend.jack.input) && + virDomainAudioIOJackIsEqual(&this->backend.jack.output, + &that->backend.jack.output); + + case VIR_DOMAIN_AUDIO_TYPE_OSS: + return virDomainAudioIOOSSIsEqual(&this->backend.oss.input, + &that->backend.oss.input) && + virDomainAudioIOOSSIsEqual(&this->backend.oss.output, + &that->backend.oss.output) && + this->backend.oss.tryMMap == that->backend.oss.tryMMap && + this->backend.oss.exclusive == that->backend.oss.exclusive && + this->backend.oss.dspPolicySet == that->backend.oss.dspPolicySet && + this->backend.oss.dspPolicy == that->backend.oss.dspPolicy; + + case VIR_DOMAIN_AUDIO_TYPE_PULSEAUDIO: + return virDomainAudioIOPulseAudioIsEqual(&this->backend.pulseaudio.input, + &that->backend.pulseaudio.input) && + virDomainAudioIOPulseAudioIsEqual(&this->backend.pulseaudio.output, + &that->backend.pulseaudio.output) && + STREQ_NULLABLE(this->backend.pulseaudio.serverName, + that->backend.pulseaudio.serverName); + + case VIR_DOMAIN_AUDIO_TYPE_SDL: + return virDomainAudioIOSDLIsEqual(&this->backend.sdl.input, + &that->backend.sdl.input) && + virDomainAudioIOSDLIsEqual(&this->backend.sdl.output, + &that->backend.sdl.output) && + this->backend.sdl.driver == that->backend.sdl.driver; + + case VIR_DOMAIN_AUDIO_TYPE_SPICE: + return true; + + case VIR_DOMAIN_AUDIO_TYPE_FILE: + return STREQ_NULLABLE(this->backend.file.path, that->backend.file.path); + + case VIR_DOMAIN_AUDIO_TYPE_LAST: + default: + return false; + } +} + + +bool +virDomainAudioIsEqual(virDomainAudioDef *this, + virDomainAudioDef *that) +{ + return this->type == that->type && + this->id == that->id && + this->timerPeriod == that->timerPeriod && + virDomainAudioIOCommonIsEqual(&this->input, &that->input) && + virDomainAudioIOCommonIsEqual(&this->output, &that->output) && + virDomainAudioBackendIsEqual(this, that); +} + + char * virDomainObjGetMetadata(virDomainObj *vm, int type, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ab9a7d66f8..cf880ccb3c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -4015,6 +4015,9 @@ bool virDomainSoundModelSupportsCodecs(virDomainSoundDef *def); bool virDomainAudioIOCommonIsSet(virDomainAudioIOCommon *common); +bool +virDomainAudioIsEqual(virDomainAudioDef *this, + virDomainAudioDef *that); const char *virDomainChrSourceDefGetPath(virDomainChrSourceDef *chr); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9ee8fda25f..a369d9c113 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -231,6 +231,7 @@ virDomainAudioDefFree; virDomainAudioFormatTypeFromString; virDomainAudioFormatTypeToString; virDomainAudioIOCommonIsSet; +virDomainAudioIsEqual; virDomainAudioSDLDriverTypeFromString; virDomainAudioSDLDriverTypeToString; virDomainAudioTypeTypeFromString; -- 2.31.1
To support backwards live migration we must strip the default added audio element, however, we are too aggressive in doing so. We are only comparing a couple of attributes for equality, so risk stripping config that was user customized. To improve this we need to a deep comparison of the audio config. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_domain.c | 72 +++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fb203bc830..ef0a08d405 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3507,17 +3507,15 @@ qemuDomainDefSuggestDefaultAudioBackend(virQEMUDriver *driver, } static int -qemuDomainDefClearDefaultAudioBackend(virQEMUDriver *driver, - virDomainDef *def) +qemuDomainDefCreateDefaultAudioBackend(virQEMUDriver *driver, + virDomainDef *def, + virDomainAudioDef **audioout) { bool addAudio; int audioBackend; int audioSDLDriver; - virDomainAudioDef *audio; - if (def->naudios != 1) { - return 0; - } + *audioout = NULL; if (qemuDomainDefSuggestDefaultAudioBackend(driver, def, @@ -3526,21 +3524,45 @@ qemuDomainDefClearDefaultAudioBackend(virQEMUDriver *driver, &audioSDLDriver) < 0) return -1; - if (!addAudio) - return 0; + if (addAudio) { + virDomainAudioDef *audio = g_new0(virDomainAudioDef, 1); + + audio->type = audioBackend; + audio->id = 1; - audio = def->audios[0]; - if (audio->type != audioBackend) + if (audioBackend == VIR_DOMAIN_AUDIO_TYPE_SDL) + audio->backend.sdl.driver = audioSDLDriver; + + *audioout = audio; + } + + return 0; +} + + +static int +qemuDomainDefClearDefaultAudioBackend(virQEMUDriver *driver, + virDomainDef *def) +{ + virDomainAudioDef *audio = NULL; + + if (def->naudios != 1) { return 0; + } - if (audio->type == VIR_DOMAIN_AUDIO_TYPE_SDL && - audio->backend.sdl.driver != audioSDLDriver) + if (qemuDomainDefCreateDefaultAudioBackend(driver, def, &audio) < 0) + return -1; + + if (!audio) return 0; + if (virDomainAudioIsEqual(def->audios[0], audio)) { + virDomainAudioDefFree(def->audios[0]); + g_free(def->audios); + def->naudios = 0; + def->audios = NULL; + } virDomainAudioDefFree(audio); - g_free(def->audios); - def->naudios = 0; - def->audios = NULL; return 0; } @@ -3549,33 +3571,19 @@ static int qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver, virDomainDef *def) { - bool addAudio; - int audioBackend; - int audioSDLDriver; + virDomainAudioDef *audio; if (def->naudios > 0) { return 0; } - if (qemuDomainDefSuggestDefaultAudioBackend(driver, - def, - &addAudio, - &audioBackend, - &audioSDLDriver) < 0) + if (qemuDomainDefCreateDefaultAudioBackend(driver, def, &audio) < 0) return -1; - if (addAudio) { - virDomainAudioDef *audio = g_new0(virDomainAudioDef, 1); - - audio->type = audioBackend; - audio->id = 1; - + if (audio) { def->naudios = 1; def->audios = g_new0(virDomainAudioDef *, def->naudios); def->audios[0] = audio; - - if (audioBackend == VIR_DOMAIN_AUDIO_TYPE_SDL) - audio->backend.sdl.driver = audioSDLDriver; } return 0; -- 2.31.1
On 11/11/21 5:35 PM, Daniel P. Berrangé wrote:
Avoid being over-eager in stripping <audio> elements.
This still isn't ideal, because if a user happened to provide an audio config that *exactly* matches the libvirt historical default we'll still strip it.
Thinking that maybe libvirt should advertize feature flags in te migration cookies.
eg if the QEMU driver on the target host reports "explicit-audiodev", then te source host knows it doesn't need to strip it out to migrate to this host. If it doesn't report it, then it knows it must be an older libvirt version pre-dating audiodev.
Daniel P. Berrangé (2): conf: add helper for comparing virDomainAudioDef objects qemu: don't strip audio elements with user config present
src/conf/domain_conf.c | 144 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 + src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 72 +++++++++++--------- 4 files changed, 188 insertions(+), 32 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Daniel P. Berrangé -
Michal Prívozník