10:35 a.m.
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
10:35 a.m.
New subject: [libvirt PATCH 1/2] conf: add helper for comparing virDomainAudioDef objects
It is useful to be able to deeply check them for equality.
Signed-off-by: Daniel P. Berrangé <berrange(a)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
10:35 a.m.
New subject: [libvirt PATCH 2/2] qemu: don't strip audio elements with user config present
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(a)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
4:06 a.m.
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(a)redhat.com>
Michal
1141
days inactive
1142
days old
3 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Michal Prívozník