Re: [libvirt] [RFC] externall (pull) backup API
ping On 14.11.2017 18:38, Nikolay Shirokovskiy wrote: > Table of contents. > > I Preface > > 1. Fleece API > 2. Export API > 3. Incremental backups > 4. Other hypervisors > > II Links > > > > > I Preface > > This is a RFC for external (or pull) backup API in libvirt. There was a > series [1] > with more limited API scope and functionality for this kind of backup API. > Besides other issues the series was abandoned as qemu blockdev-del command has > experimental status at that time. There is also a long pending RFC series for > internal (or push) backup API [2] which however has not much in comman with > this RFC. Also there is RFC with overall agreement to having a backup API in > libvirt [3]. > > The aim of external backup API is to provide means for 3d party application to > read/write domain disks as block devices for the purpuse of backup. Disk is > read on backup operation and in case of active domain is presented at some > point in time (preferable in some guest consistent state). Disk is written on > restore operation. > > As to providing disk state at some point in time one can use existing disks > snapshots for this purpose. However this RFC introduces API to leverage image > fleecing (blockdev-backup command) instead. Image fleecing is somewhat inverse > to snapshots. In case of snapshots writes go to top image thus backing image > stays constant, in case of fleecing writes go to same image as before but old > data is previously popped out to fleece image which have original image as > backing. As a result fleece image became disk snapshot. > > Another task of this API is to provide disks for read/write operations. One > could try to leverage libvirt stream API for this purpose but AFAIK clients > want random access to disks data which is not what stream API suitable for. > I'm not sure what is costs of adding block API to libvirt, particularly what > it > costs to make it effective implementation at RPC level thus this RFC add means > to export disks data thru existing block interfaces. For qemu it is NBD. > > > > 1. Fleece API > > So the below API is to provide means to start/stop/query disk image fleecing. > I use BlockSnaphost name for this operation. Other options are Fleecing, > BlockFleecing, > TempBlockSnapshot etc. > > /* Start fleecing */ > virDomainBlockSnapshotPtr > virDomainBlockSnapshotCreateXML(virDomainPtr domain, > const char *xmlDesc, > unsigned int flags); > > /* Stop fleecing */ > int > virDomainBlockSnapshotDelete(virDomainBlockSnapshotPtr snapshot, > unsigned int flags); > > /* List active fleecings */ > virDomainBlockSnapshotList(virDomainPtr domain, >virDomainBlockSnapshotPtr **snaps, >unsigned int flags); > > /* Get fleecing description */ > char* > virDomainBlockSnapshotGetXMLDesc(virDomainBlockSnapshotPtr snapshot, > unsigned int flags); > > /* Get fleecing by name */ > virDomainBlockSnapshotPtr > virDomainBlockSnapshotLookupByName(virDomainPtr domain, >const char *name); > > > Here is a minimal block snapshot xml description to feed creating function: > > > > > > > > > > > Below is an example of what getting description function should provide upon > successful block snaphost creation. The difference with the above xml is that > name element (it can be specified on creation as well) and aliases are > generated. Aliases will be useful later to identify block devices on exporting > thru nbd. > > > 5768a388-c1c4-414c-ac4e-eab216ba7c0c > > > > > > > > > > > > > 2. Export API > > During backup operation we need to provide read access to fleecing image. This > is done thru qemu process nbd server. We just need to specify the disks to > export. > > /* start block export */ > int > virDomainBlockExportStart(virDomainPtr domain, > const char *xmlDesc, > unsigned int flags); > > /* stop block export */ > int > virDomainBlockExportStop(virDomainPtr domain, > const char *diskName, > unsigned int flags); > > Here is an example of xml for starting function: > > > > > > > qemu nbd server is started upon first disk export start and shutted down upon > last disk export stop. Another option is to control ndb server explicitly. One > way to do it is to consider ndb server a new device so to start/stop/update > ndb > server we can use attach/detach/update device functions. Then in block export > start we need to refer to this device somehow. This can be a generated > name/uuid or type/address pair. Actually this approach to expose ndb server > looks more natural to me even it includes more management from client side. > I am not suggesting it in the
Re: [libvirt] [PATCH 5/5] news: Document which drivers support NUMA distances
On 11/14/2017 09:47 AM, Michal Privoznik wrote: > Signed-off-by: Michal Privoznik > --- > docs/news.xml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Drat - should have noted this in review of 4/5... I think you'll also need a change to docs/formatdomain.html.in - see the last tidbit in commit id '74119a03f' Describing distances between NUMA cells is currently only supported by Xen. If no distances are given to describe the SLIT data between different cells, it will default to a scheme using 10 for local and 20 for remote distances. For this though... Reviewed-by: John Ferlan John > diff --git a/docs/news.xml b/docs/news.xml > index 3966710ee..502679917 100644 > --- a/docs/news.xml > +++ b/docs/news.xml > @@ -43,7 +43,7 @@ >A NUMA hardware architecture supports the notion of distances >between NUMA cells. This can now be specified using the >element within the NUMA cell > - configuration. > + configuration. Drivers which support this include Xen and QEMU. > > > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] qemu: Support setting NUMA distances
On 11/14/2017 09:47 AM, Michal Privoznik wrote:
> Since we already have such support for libxl all we need is qemu
> driver adjustment. And a test case.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_command.c| 36 +++-
> .../qemuxml2argv-numatune-distances.args | 63 +
> .../qemuxml2argv-numatune-distances.xml| 65
> ++
> tests/qemuxml2argvtest.c | 2 +
> 4 files changed, 165 insertions(+), 1 deletion(-)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.xml
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index eb72db33b..8b9daaea3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7675,7 +7675,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
> virCommandPtr cmd,
> qemuDomainObjPrivatePtr priv)
> {
> -size_t i;
> +size_t i, j;
> virQEMUCapsPtr qemuCaps = priv->qemuCaps;
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> char *cpumask = NULL, *tmpmask = NULL, *next = NULL;
> @@ -7685,6 +7685,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
> int ret = -1;
> size_t ncells = virDomainNumaGetNodeCount(def->numa);
> const long system_page_size = virGetSystemPageSizeKB();
> +bool numa_distances = false;
>
> if (virDomainNumatuneHasPerNodeBinding(def->numa) &&
> !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
> @@ -7793,6 +7794,39 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>
> virCommandAddArgBuffer(cmd, &buf);
> }
> +
> +/* If NUMA node distance is specified for at least one pair
> + * of nodes, we have to specify all the distances. Even
> + * though they might be the default ones. */
> +for (i = 0; i < ncells; i++) {
> +for (j = 0; j < ncells; j++) {
> +if (!virDomainNumaNodeDistanceSpecified(def->numa, i, j))
> +continue;
> +
> +numa_distances = true;
> +
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_DIST)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("setting NUMA distances is not "
> + "supported with this qemu"));
> +goto cleanup;
> +}
> +}
> +}
Not sure I understand the need for the above double loop Even with
your adjustment...
It would seem that all that's necessary is
for (i < 0; i < ncells; i++) {
if (numa->mem_nodes[i].ndistances > 0)
break;
}
if (i < ncells) {
CapsCheck
The next double for loop now would seem to apply without
the need for numa_distances boolean.
}
Or am I off in the weeds?
John
> +
> +if (numa_distances) {
> +for (i = 0; i < ncells; i++) {
> +for (j = 0; j < ncells; j++) {
> +size_t distance = virDomainNumaGetNodeDistance(def->numa, i,
> j);
> +
> +virCommandAddArg(cmd, "-numa");
> +virBufferAsprintf(&buf, "dist,src=%zu,dst=%zu,val=%zu", i,
> j, distance);
> +
> +virCommandAddArgBuffer(cmd, &buf);
> +}
> +}
> +}
> +
> ret = 0;
>
> cleanup:
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args
> b/tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args
> new file mode 100644
> index 0..23b66246c
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args
> @@ -0,0 +1,63 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-x86_64 \
> +-name QEMUGuest \
> +-S \
> +-M xenfv \
> +-m 12288 \
> +-smp 12,sockets=12,cores=1,threads=1 \
> +-numa node,nodeid=0,cpus=0,cpus=11,mem=2048 \
> +-numa node,nodeid=1,cpus=1,cpus=10,mem=2048 \
> +-numa node,nodeid=2,cpus=2,cpus=9,mem=2048 \
> +-numa node,nodeid=3,cpus=3,cpus=8,mem=2048 \
> +-numa node,nodeid=4,cpus=4,cpus=7,mem=2048 \
> +-numa node,nodeid=5,cpus=5-6,mem=2048 \
> +-numa dist,src=0,dst=0,val=10 \
> +-numa dist,src=0,dst=1,val=21 \
> +-numa dist,src=0,dst=2,val=31 \
> +-numa dist,src=0,dst=3,val=41 \
> +-numa dist,src=0,dst=4,val=51 \
> +-numa dist,src=0,dst=5,val=61 \
> +-numa dist,src=1,dst=0,val=21 \
> +-numa dist,src=1,dst=1,val=10 \
> +-numa dist,src=1,dst=2,val=21 \
> +-numa dist,src=1,dst=3,val=31 \
> +-numa dist,src=1,dst=4,val=41 \
> +-numa dist,src=1,dst=5,val=51 \
> +-numa dist,src=2,dst=0,val=31 \
> +-numa dist,src=2,dst=1,val=21 \
> +-numa dist,src=2,dst=2,val=10 \
> +-numa dist,src=2,dst=3,val=21 \
> +-numa dist,src=2,dst=4,val=31 \
> +-numa dist,src=2,dst=5,val=41 \
> +-numa dist,src=3,dst=0,val=41 \
> +-numa dist,src=3,dst=1,val=31 \
> +-numa dist,src=3,dst
Re: [libvirt] [PATCH 3/5] qemu_capabilities: Introcude QEMU_CAPS_NUMA_DIST
On 11/14/2017 09:47 AM, Michal Privoznik wrote: > This capability says if qemu is capable of specifying distances > between NUMA nodes on the command line. Unfortunately, there's no > real way to check this and thus we have to go with version check. > QEMU introduced this in 0f203430dd8 (and friend) which was > released in 2.10.0. > > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_capabilities.c | 5 + > src/qemu/qemu_capabilities.h | 1 + > tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + > tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml| 1 + > 7 files changed, 11 insertions(+) > With the obvious merge to the ever changing current capabilities at the top of the tree... Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] numa: Introduce virDomainNumaNodeDistanceSpecified
On 11/14/2017 09:47 AM, Michal Privoznik wrote:
> The function returns true/false depending on distance
> configuration being present in the domain XML.
>
> Signed-off-by: Michal Privoznik
> ---
> src/conf/numa_conf.c | 13 +
> src/conf/numa_conf.h | 4
> src/libvirt_private.syms | 1 +
> 3 files changed, 18 insertions(+)
>
> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
> index 5f0b3f9ed..6a42777e2 100644
> --- a/src/conf/numa_conf.c
> +++ b/src/conf/numa_conf.c
> @@ -1137,6 +1137,19 @@ virDomainNumaSetNodeCount(virDomainNumaPtr numa,
> size_t nmem_nodes)
> return numa->nmem_nodes;
> }
>
Two blank lines here too.
> +bool
> +virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa,
> + size_t node,
> + size_t sibling)
> +{
> +return node < numa->nmem_nodes &&
> +sibling < numa->nmem_nodes &&
> +numa->mem_nodes[node].distances &&
> +numa->mem_nodes[node].distances[sibling].value != LOCAL_DISTANCE &&
> +numa->mem_nodes[node].distances[sibling].value != REMOTE_DISTANCE;
> +}
> +
> +
According to how I read commit message '74119a03' it *is* possible to
set the @value to the same value as LOCAL_DISTANCE(10) or
REMOTE_DISTANCE(20) - so using that as a comparison for whether it was
specified would seem to be wrong.
Still if a distance *is* provided, then it seems that 'id' and 'value'
are also required to be provided or defaulted. That means what seems to
matter regarding whether something was provided is if the *.value and/or
"*.cellid are zero
At least that's how I'm reading virDomainNumaDefNodeDistanceParseXML
John
> size_t
> virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
> size_t node,
> diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h
> index 4655de3aa..1d2e605b6 100644
> --- a/src/conf/numa_conf.h
> +++ b/src/conf/numa_conf.h
> @@ -87,6 +87,10 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr
> numatune,
>
> size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa);
>
> +bool virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa,
> +size_t node,
> +size_t sibling)
> +ATTRIBUTE_NONNULL(1);
> size_t virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
> size_t node,
> size_t sibling)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5a4d50471..779bab7a3 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -725,6 +725,7 @@ virDomainNumaGetNodeDistance;
> virDomainNumaGetNodeMemoryAccessMode;
> virDomainNumaGetNodeMemorySize;
> virDomainNumaNew;
> +virDomainNumaNodeDistanceSpecified;
> virDomainNumaSetNodeCount;
> virDomainNumaSetNodeCpumask;
> virDomainNumaSetNodeDistance;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] virDomainNumaGetNodeDistance: Fix input arguments validation
On 11/14/2017 09:47 AM, Michal Privoznik wrote: > There's no point in checking if numa->mem_nodes[node].ndistances > is set if we check for numa->mem_nodes[node].distances. However, > it makes sense to check if the sibling node caller passed falls > within boundaries. > > Signed-off-by: Michal Privoznik > --- > src/conf/numa_conf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c > index 7bba4120b..5f0b3f9ed 100644 > --- a/src/conf/numa_conf.c > +++ b/src/conf/numa_conf.c > @@ -1154,7 +1154,7 @@ virDomainNumaGetNodeDistance(virDomainNumaPtr numa, > */ > if (!distances || > !distances[cellid].value || > -!numa->mem_nodes[node].ndistances) > +node >= numa->nmem_nodes) If @distances can only be set if "node < numa->nmem_nodes", then how could "node >= numa->nmem_nodes" ever be true and @distances be non NULL? IOW: I see no need for the check... This former condition also trips across my "favorite" condition check of "if !intValue" substituting for "if intValue == 0" . BTW: I do think there is a memory leak @distances entries are not VIR_FREE'd in virDomainNumaFree. I was looking for instances where ndistances maybe have been forgotten to be set to 0 even though distances was cleared. I can send a patch or you can for that if you want - IDC... There's a couple of other cleanups I'd like to see w/r/t using (!*ndistances) and how the @*distance are set to ldist/rdist outside of the if condition that allocated, but those are type A type things ;-) John > return (node == cellid) ? LOCAL_DISTANCE : REMOTE_DISTANCE; > > return distances[cellid].value; > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 01/21] qemu: Introduce qemuDomainChrDefPostParse()
On Tue, Nov 21, 2017 at 05:42:11PM +0100, Andrea Bolognani wrote: > Having a separate function for char device handling is better than > adding even more code to qemuDomainDeviceDefPostParse(). > > Signed-off-by: Andrea Bolognani > --- > src/qemu/qemu_domain.c | 57 > ++ > 1 file changed, 34 insertions(+), 23 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] qemu: implement element for devices
On Tue, Nov 21, 2017 at 12:44:04PM -0500, John Ferlan wrote: > > > On 11/21/2017 12:12 PM, Pavel Hrdina wrote: > > On Tue, Nov 21, 2017 at 10:52:48AM -0500, John Ferlan wrote: > >> > >> > >> On 11/21/2017 08:42 AM, Pavel Hrdina wrote: > >>> On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote: > > > On 11/14/2017 08:45 AM, Pavel Hrdina wrote: > > So far we were configuring the sound output based on what graphic device > > was configured in domain XML. This had a several disadvantages, it's > > not transparent, in case of multiple graphic devices it was overwritten > > by the last one and there was no simple way how to configure this per > > domain. > > > > The new element for devices allows you to configure > > which output will be used for each domain, however QEMU has a limitation > > that all sound devices will always use the same output because it is > > configured by environment variable QEMU_AUDIO_DRV per domain. > > > > For backward compatibility we need to preserve the defaults if no output > > is specified: > > > > - for vnc graphic it's by default NONE unless "vnc_allow_host_audio" > > was enabled, in that case we use DEFAULT which means it will pass > > the environment variable visible by libvirtd > > > > - for sdl graphic by default we pass the environment variable > > > > - for spice graphic we configure the SPICE output > > > > - if no graphic is configured we use by default NONE unless > > "nogfx_allow_host_audio" was enabled, in that case we pass > > the environment variable > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1375433 > > > > Signed-off-by: Pavel Hrdina > > --- > > docs/formatdomain.html.in | 4 ++- > > src/qemu/qemu_command.c | 84 > > +-- > > src/qemu/qemu_domain.c| 54 ++ > > src/qemu/qemu_process.c | 41 +++ > > 4 files changed, 135 insertions(+), 48 deletions(-) > > > > Is there any way to make less things happen in one patch? Maybe even > the PostParse, Prepare, and Validate together? > >>> > >>> It would be probably possible to split this patch into two separate > >>> changes: > >>> > >>> 1. move the code out of command line generation into > >>> qemuProcessPrepareSound() > >>> > >>> 2. the rest of this patch > >>> > I need to look at this one with fresh eyes in the morning - it's > confusing at best especially since I don't find the functions self > documenting. > > I'm having trouble with the settings between PostParse and Prepare as > well as setting a default output which essentially changes an optional > parameter into a required one, doesn't it? I'm sure there's a harder and > even more confusing way to do things ;-). > >>> > >>> The PostParse function tries to find the first sound device with output > >>> configured (the first for loop) and sets this output to all other sound > >>> devices without any output configured (the second for loop). This is > >>> executed while parsing domain XML and implements the new feature. > >> > >> Understood, but it also changes each configured sound device that didn't > >> have defined to have the value from the one that did > >> have it. > >> > >> That would then be saved - something that would have been shown in the > >> qemuxml2xmltest output, e.g the multi output from patch 6 would be: > >> > >> > >> > >>>> function='0x0'/> > >> > >> > >> > >>>> function='0x0'/> > >> > >> > > > > Which part of the PostParse code does that? If you configure the domain > > XML like this: > > > > > > > > > > The resulting config XML saved to disk would be: > > > > > > < > unction='0x0'/> > > > > > > < > unction='0x0'/> > > > > > >From patch 6 I used: > > tests/qemuxml2argvdata/qemuxml2argv-sound-multi-pa-output-spice.xml > > which has > > > > > Right, it was not clear what you meant, next time please include this input XML right away, it will save both of us some time to understand each other :). I see your point that this update could have been done in Prepare function. I chose this implementation to make it clear that there is the limitation with QEMU, but it would also work to put it into Prepare function. That way if QEMU introduces support to configure different output for each sound device users would benefit from it automatically for existing domains once we add it also into libvirt. > > > > One of the reasons why I didn't add qemuxml2xml tests is that only the > > offline part is somehow working, but the active and status part of that > > test is broken and doesn't reflect how libvirt changes the active and > > status XML. > > >
Re: [libvirt] [PATCH 5/6] qemu: implement element for devices
On 11/21/2017 12:12 PM, Pavel Hrdina wrote: > On Tue, Nov 21, 2017 at 10:52:48AM -0500, John Ferlan wrote: >> >> >> On 11/21/2017 08:42 AM, Pavel Hrdina wrote: >>> On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote: On 11/14/2017 08:45 AM, Pavel Hrdina wrote: > So far we were configuring the sound output based on what graphic device > was configured in domain XML. This had a several disadvantages, it's > not transparent, in case of multiple graphic devices it was overwritten > by the last one and there was no simple way how to configure this per > domain. > > The new element for devices allows you to configure > which output will be used for each domain, however QEMU has a limitation > that all sound devices will always use the same output because it is > configured by environment variable QEMU_AUDIO_DRV per domain. > > For backward compatibility we need to preserve the defaults if no output > is specified: > > - for vnc graphic it's by default NONE unless "vnc_allow_host_audio" > was enabled, in that case we use DEFAULT which means it will pass > the environment variable visible by libvirtd > > - for sdl graphic by default we pass the environment variable > > - for spice graphic we configure the SPICE output > > - if no graphic is configured we use by default NONE unless > "nogfx_allow_host_audio" was enabled, in that case we pass > the environment variable > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1375433 > > Signed-off-by: Pavel Hrdina > --- > docs/formatdomain.html.in | 4 ++- > src/qemu/qemu_command.c | 84 > +-- > src/qemu/qemu_domain.c| 54 ++ > src/qemu/qemu_process.c | 41 +++ > 4 files changed, 135 insertions(+), 48 deletions(-) > Is there any way to make less things happen in one patch? Maybe even the PostParse, Prepare, and Validate together? >>> >>> It would be probably possible to split this patch into two separate >>> changes: >>> >>> 1. move the code out of command line generation into >>> qemuProcessPrepareSound() >>> >>> 2. the rest of this patch >>> I need to look at this one with fresh eyes in the morning - it's confusing at best especially since I don't find the functions self documenting. I'm having trouble with the settings between PostParse and Prepare as well as setting a default output which essentially changes an optional parameter into a required one, doesn't it? I'm sure there's a harder and even more confusing way to do things ;-). >>> >>> The PostParse function tries to find the first sound device with output >>> configured (the first for loop) and sets this output to all other sound >>> devices without any output configured (the second for loop). This is >>> executed while parsing domain XML and implements the new feature. >> >> Understood, but it also changes each configured sound device that didn't >> have defined to have the value from the one that did >> have it. >> >> That would then be saved - something that would have been shown in the >> qemuxml2xmltest output, e.g the multi output from patch 6 would be: >> >> >> >> > function='0x0'/> >> >> >> >> > function='0x0'/> >> >> > > Which part of the PostParse code does that? If you configure the domain > XML like this: > > > > > The resulting config XML saved to disk would be: > > > < unction='0x0'/> > > > < unction='0x0'/> > >From patch 6 I used: tests/qemuxml2argvdata/qemuxml2argv-sound-multi-pa-output-spice.xml which has > > One of the reasons why I didn't add qemuxml2xml tests is that only the > offline part is somehow working, but the active and status part of that > test is broken and doesn't reflect how libvirt changes the active and > status XML. > I didn't pay close attention to which was which, just that some output xml was generated by the regenerate. John >> I also note that comes after ... not that it matters, >> but my brain recollects that is generally last for most >> elements, although not required to be last - it just is generally. > > Good point, I'll fix that. > >> In any case, I'm not sure it's "right" to change/add that output. It >> should be simple enough to ignore those that don't define some output. >> That was my point about making an optional element required. >> >> Being able to provide/determine some default when no sound device has an >> output defined would thus become the "job" of the Prepare API I think. > > Well, that's how it works with this patches. > >>> >>> The Prepare function is executed only while starting domain and >>> basically supplements the code removed from bui
Re: [libvirt] [PATCH 5/6] qemu: implement element for devices
On Tue, Nov 21, 2017 at 10:52:48AM -0500, John Ferlan wrote: > > > On 11/21/2017 08:42 AM, Pavel Hrdina wrote: > > On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote: > >> > >> > >> On 11/14/2017 08:45 AM, Pavel Hrdina wrote: > >>> So far we were configuring the sound output based on what graphic device > >>> was configured in domain XML. This had a several disadvantages, it's > >>> not transparent, in case of multiple graphic devices it was overwritten > >>> by the last one and there was no simple way how to configure this per > >>> domain. > >>> > >>> The new element for devices allows you to configure > >>> which output will be used for each domain, however QEMU has a limitation > >>> that all sound devices will always use the same output because it is > >>> configured by environment variable QEMU_AUDIO_DRV per domain. > >>> > >>> For backward compatibility we need to preserve the defaults if no output > >>> is specified: > >>> > >>> - for vnc graphic it's by default NONE unless "vnc_allow_host_audio" > >>> was enabled, in that case we use DEFAULT which means it will pass > >>> the environment variable visible by libvirtd > >>> > >>> - for sdl graphic by default we pass the environment variable > >>> > >>> - for spice graphic we configure the SPICE output > >>> > >>> - if no graphic is configured we use by default NONE unless > >>> "nogfx_allow_host_audio" was enabled, in that case we pass > >>> the environment variable > >>> > >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1375433 > >>> > >>> Signed-off-by: Pavel Hrdina > >>> --- > >>> docs/formatdomain.html.in | 4 ++- > >>> src/qemu/qemu_command.c | 84 > >>> +-- > >>> src/qemu/qemu_domain.c| 54 ++ > >>> src/qemu/qemu_process.c | 41 +++ > >>> 4 files changed, 135 insertions(+), 48 deletions(-) > >>> > >> > >> Is there any way to make less things happen in one patch? Maybe even > >> the PostParse, Prepare, and Validate together? > > > > It would be probably possible to split this patch into two separate > > changes: > > > > 1. move the code out of command line generation into > > qemuProcessPrepareSound() > > > > 2. the rest of this patch > > > >> I need to look at this one with fresh eyes in the morning - it's > >> confusing at best especially since I don't find the functions self > >> documenting. > >> > >> I'm having trouble with the settings between PostParse and Prepare as > >> well as setting a default output which essentially changes an optional > >> parameter into a required one, doesn't it? I'm sure there's a harder and > >> even more confusing way to do things ;-). > > > > The PostParse function tries to find the first sound device with output > > configured (the first for loop) and sets this output to all other sound > > devices without any output configured (the second for loop). This is > > executed while parsing domain XML and implements the new feature. > > Understood, but it also changes each configured sound device that didn't > have defined to have the value from the one that did > have it. > > That would then be saved - something that would have been shown in the > qemuxml2xmltest output, e.g the multi output from patch 6 would be: > > > >function='0x0'/> > > > >function='0x0'/> > > Which part of the PostParse code does that? If you configure the domain XML like this: The resulting config XML saved to disk would be: < < One of the reasons why I didn't add qemuxml2xml tests is that only the offline part is somehow working, but the active and status part of that test is broken and doesn't reflect how libvirt changes the active and status XML. > I also note that comes after ... not that it matters, > but my brain recollects that is generally last for most > elements, although not required to be last - it just is generally. Good point, I'll fix that. > In any case, I'm not sure it's "right" to change/add that output. It > should be simple enough to ignore those that don't define some output. > That was my point about making an optional element required. > > Being able to provide/determine some default when no sound device has an > output defined would thus become the "job" of the Prepare API I think. Well, that's how it works with this patches. > > > > The Prepare function is executed only while starting domain and > > basically supplements the code removed from building command line. > > It prepares only live definition that is about to be started so > > the qemu command line code can only take the definition and convert > > it into command line without any logic or without modifying the > > live definition. > > > > Right - these are the live entries not the config/saved defs - so to > that degree altering things does make some sense. However, your cho
Re: [libvirt] Redesigning Libvirt: Adopting use of a safe language
On 11/20/2017 09:25 AM, Daniel P. Berrange wrote: When I worked in OpenStack it was a constant battle to get people to consider enhancements to libvirt instead of reinventing it in Python. It was a hard sell because most python dev just didn't want to use C at all because it has a high curve to contributors, even if libvirt as a community is welcoming. As a result OpenStack pretty much reinvented its own hypervisor agnostic API for esx, hyperv, xenapi and KVM instead of enhancing libvirt's support for esx, hyperv or xenapi. To be fair, there's also the issue that getting a change into any external project and packaged into all the distros is more unpredictable (and may take longer) than implementing the same thing in their own project. RHEL (just as an example) has been updating libvirt roughly once a year for the past couple years. Chris -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 20/21] conf: convert sclp/sclplm as
On Tue, 2017-11-21 at 17:42 +0100, Andrea Bolognani wrote:
> +static void
> +virDomainChrConsoleTargetTypeToSerial(virDomainChrConsoleTargetType type,
> + int *retType, int *retModel)
The last two arguments should be each on a separate line.
I'd also prefer the names 'targetType' and 'targetModel'.
> @@ -4014,7 +4048,9 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def,
> }
> if (def->nconsoles > 0 && def->os.type == VIR_DOMAIN_OSTYPE_HVM &&
> (def->consoles[0]->targetType ==
> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ||
> - def->consoles[0]->targetType ==
> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE)) {
> + def->consoles[0]->targetType ==
> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE ||
> + (def->consoles[0]->targetType ==
> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLP &&
> + (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE {
VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLPLM should be included in the
check above, or s with will not
be converted into s.
> @@ -4027,6 +4063,8 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def,
>
> /* create the serial port definition from the console definition */
> if (def->nserials == 0) {
> +virDomainChrConsoleTargetType type =
> def->consoles[0]->targetType;
> +
> if (VIR_APPEND_ELEMENT(def->serials,
> def->nserials,
> def->consoles[0]) < 0)
> @@ -4034,7 +4072,9 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def,
>
> /* modify it to be a serial port */
> def->serials[0]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
> -def->serials[0]->targetType =
> VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE;
> +virDomainChrConsoleTargetTypeToSerial(type,
> +
> &def->serials[0]->targetType,
> +
> &def->serials[0]->targetModel);
Drop 'type' and just use 'def->consoles[0]->targetType' here.
Everything else looks good, so my R-b stands. I can fix all of the
above before pushing (assuming a respin won't be necessary).
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 18/21] conf: add VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP
From: Pino Toscano
Introduce specific a target types with two models for the console
devices (sclp and sclplm) used in s390 and s390x guests, so isa-serial
is no more used for them.
This makes usable on s390 and s390x guests, with at most only
a single sclpconsole and one sclplmconsole devices usable in a single
guest (due to limitations in QEMU, which will enforce already at
runtime).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1449265
Signed-off-by: Pino Toscano
---
docs/formatdomain.html.in | 2 +-
docs/schemas/domaincommon.rng | 3 ++
src/conf/domain_conf.c | 5
src/conf/domain_conf.h | 3 ++
src/qemu/qemu_command.c| 16 ++
src/qemu/qemu_domain.c | 30 +++
src/qemu/qemu_domain_address.c | 1 +
.../qemuxml2argv-s390-serial-2.args| 24 +++
.../qemuxml2argv-s390-serial-2.xml | 19
.../qemuxml2argv-s390-serial-console.args | 25
.../qemuxml2argv-s390-serial-console.xml | 15 ++
.../qemuxml2argvdata/qemuxml2argv-s390-serial.args | 22 ++
.../qemuxml2argvdata/qemuxml2argv-s390-serial.xml | 14 +
tests/qemuxml2argvtest.c | 16 ++
.../qemuxml2xmlout-s390-serial-2.xml | 33 +
.../qemuxml2xmlout-s390-serial-console.xml | 34 ++
.../qemuxml2xmlout-s390-serial.xml | 28 ++
tests/qemuxml2xmltest.c| 6
18 files changed, 295 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-serial-2.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-serial-2.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-serial-console.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-serial-console.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-serial.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-serial.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-serial-2.xml
create mode 100644
tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-serial-console.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-serial.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 1efded6be..fd85b7633 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6532,7 +6532,7 @@ qemu-kvm -net nic,model=? /dev/null
type attribute since 1.0.2
which can be used to pick between isa, usb,
pci and, since 3.10.0,
- spapr-vio and system.
+ spapr-vio, system and sclp.
Some values are not compatible with all architecture and machine types;
if the value is missing altogether, libvirt will try to pick an
appropriate default.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 949ad38ac..fe90c78a8 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3587,6 +3587,7 @@
pci
spapr-vio
system
+sclp
isa-serial
usb-serial
@@ -3604,6 +3605,8 @@
pci-serial
spapr-vty
pl011
+ sclpconsole
+ sclplmconsole
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7b35fbd3d..1dcd0e91a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -454,6 +454,7 @@ VIR_ENUM_IMPL(virDomainChrSerialTarget,
"pci",
"spapr-vio",
"system",
+ "sclp",
);
VIR_ENUM_IMPL(virDomainChrChannelTarget,
@@ -483,6 +484,8 @@ VIR_ENUM_IMPL(virDomainChrSerialTargetModel,
"pci-serial",
"spapr-vty",
"pl011",
+ "sclpconsole",
+ "sclplmconsole",
);
VIR_ENUM_IMPL(virDomainChrDevice, VIR_DOMAIN_CHR_DEVICE_TYPE_LAST,
@@ -4057,6 +4060,7 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def)
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: {
/* Create a stub console to match the serial port.
@@ -24099,6 +24103,7 @@ virDomainChrTargetDefFormat(virBufferPtr buf,
break;
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
[libvirt] [PATCH v2 21/21] qemu: switch s390/s390x default console back to serial
From: Pino Toscano
Now that and on s390/s390x behave a bit more like the
other architectures, remove this extra differentation, and use sclp
console by default for new guests. New virtio consoles can still be
added, and it is actually needed because of the limited number of
instances for sclp and sclplm.
This reverts commit b1c88c14764e0b043a269d454a83a6ac7af34eac, whose
reasons are not totally clear.
Signed-off-by: Pino Toscano
Reviewed-by: Andrea Bolognani
Reviewed-by: Bjoern Walk
---
src/qemu/qemu_domain.c | 7 ---
tests/qemuxml2argvdata/qemuxml2argv-s390-serial-console.args| 5 +
tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-defaultconsole.xml | 8 ++--
tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-serial-console.xml | 6 --
4 files changed, 7 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 21eb371e7..49a613675 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4224,13 +4224,6 @@ qemuDomainChrDefPostParse(virDomainChrDefPtr chr,
virQEMUDriverPtr driver,
unsigned int parseFlags)
{
-/* set the default console type for S390 arches */
-if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
-chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE &&
-ARCH_IS_S390(def->os.arch)) {
-chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
-}
-
/* Historically, isa-serial and the default matched, so in order to
* maintain backwards compatibility we map them here. The actual default
* will be picked below based on the architecture and machine type. */
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-s390-serial-console.args
b/tests/qemuxml2argvdata/qemuxml2argv-s390-serial-console.args
index c405fb59e..20968f794 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-s390-serial-console.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-s390-serial-console.args
@@ -18,8 +18,5 @@ QEMU_AUDIO_DRV=none \
server,nowait \
-mon chardev=charmonitor,id=monitor,mode=readline \
-boot c \
--device virtio-serial-ccw,id=virtio-serial0,devno=fe.0. \
-chardev pty,id=charserial0 \
--device sclpconsole,chardev=charserial0,id=serial0 \
--chardev pty,id=charconsole1 \
--device virtconsole,chardev=charconsole1,id=console1
+-device sclpconsole,chardev=charserial0,id=serial0
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-defaultconsole.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-defaultconsole.xml
index 7eb1a765a..677dd11c1 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-defaultconsole.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-defaultconsole.xml
@@ -14,9 +14,13 @@
destroy
/usr/bin/qemu-system-s390x
-
+
+
+
+
+
-
+
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-serial-console.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-serial-console.xml
index f8f5dec4a..98395c2d2 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-serial-console.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-s390-serial-console.xml
@@ -14,9 +14,6 @@
destroy
/usr/bin/qemu-system-s390x
-
-
-
@@ -25,9 +22,6 @@
-
-
-
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 19/21] conf: pass parseFlags down to virDomainDefAddConsoleCompat
From: Pino Toscano
Signed-off-by: Pino Toscano
Reviewed-by: Andrea Bolognani
---
src/conf/domain_conf.c | 10 ++
src/conf/domain_conf.h | 3 ++-
src/vz/vz_sdk.c| 2 +-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1dcd0e91a..c4497ab0f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3980,7 +3980,8 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
static int
-virDomainDefAddConsoleCompat(virDomainDefPtr def)
+virDomainDefAddConsoleCompat(virDomainDefPtr def,
+ unsigned int parseFlags ATTRIBUTE_UNUSED)
{
size_t i;
@@ -4941,7 +4942,7 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
if (virDomainDefPostParseTimer(def) < 0)
return -1;
-if (virDomainDefAddImplicitDevices(def) < 0)
+if (virDomainDefAddImplicitDevices(def, data->parseFlags) < 0)
return -1;
if (def->nvideos != 0) {
@@ -22016,9 +22017,10 @@ virDomainDefAddImplicitVideo(virDomainDefPtr def)
}
int
-virDomainDefAddImplicitDevices(virDomainDefPtr def)
+virDomainDefAddImplicitDevices(virDomainDefPtr def,
+ unsigned int parseFlags)
{
-if (virDomainDefAddConsoleCompat(def) < 0)
+if (virDomainDefAddConsoleCompat(def, parseFlags) < 0)
return -1;
if (virDomainDefAddImplicitControllers(def) < 0)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b06f40ab8..0c2daa81a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2949,7 +2949,8 @@ bool virDomainDefCheckABIStabilityFlags(virDomainDefPtr
src,
virDomainXMLOptionPtr xmlopt,
unsigned int flags);
-int virDomainDefAddImplicitDevices(virDomainDefPtr def);
+int virDomainDefAddImplicitDevices(virDomainDefPtr def,
+ unsigned int parseFlags);
virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(const virDomainDef *def,
unsigned int iothread_id);
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index eea5f6fc6..35522f46c 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1932,7 +1932,7 @@ prlsdkLoadDomain(vzDriverPtr driver,
if (prlsdkGetDomainState(dom, sdkdom, &domainState) < 0)
goto error;
-if (!IS_CT(def) && virDomainDefAddImplicitDevices(def) < 0)
+if (!IS_CT(def) && virDomainDefAddImplicitDevices(def, 0) < 0)
goto error;
if (def->ngraphics > 0) {
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 20/21] conf: convert sclp/sclplm as
From: Pino Toscano
In case we are allowed to break the ABI of a s390/s390x guest, then
convert the first sclp/sclplm console from to , just
like it is done on other architectures.
Signed-off-by: Pino Toscano
Reviewed-by: Andrea Bolognani
Reviewed-by: Bjoern Walk
---
src/conf/domain_conf.c | 46 --
.../qemuxml2argv-s390-console2serial.args | 22 +++
.../qemuxml2argv-s390-console2serial.xml | 19 +
tests/qemuxml2argvtest.c | 6 +++
4 files changed, 90 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-console2serial.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-console2serial.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c4497ab0f..027e91bb6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3979,9 +3979,39 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
}
+static void
+virDomainChrConsoleTargetTypeToSerial(virDomainChrConsoleTargetType type,
+ int *retType, int *retModel)
+{
+switch (type) {
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLP:
+*retType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP;
+*retModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE;
+break;
+
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLPLM:
+*retType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP;
+*retModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE;
+break;
+
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST:
+*retType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE;
+*retModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE;
+break;
+}
+}
+
+
static int
virDomainDefAddConsoleCompat(virDomainDefPtr def,
- unsigned int parseFlags ATTRIBUTE_UNUSED)
+ unsigned int parseFlags)
{
size_t i;
@@ -3998,6 +4028,10 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def,
*
* We then fill def->consoles[0] with a stub just so we get sequencing
* correct for consoles > 0
+ *
+ * sclp/sclplm consoles (in s390 and s390x guests) are converted to serial
+ * only when we can update the ABI of the guest, to avoid breaking
+ * migrations to old libvirt.
*/
/* Only the first console (if there are any) can be of type serial,
@@ -4014,7 +4048,9 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def,
}
if (def->nconsoles > 0 && def->os.type == VIR_DOMAIN_OSTYPE_HVM &&
(def->consoles[0]->targetType ==
VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ||
- def->consoles[0]->targetType ==
VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE)) {
+ def->consoles[0]->targetType ==
VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE ||
+ (def->consoles[0]->targetType ==
VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLP &&
+ (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE {
/* If there isn't a corresponding serial port:
* - create one and set, the console to be an alias for it
@@ -4027,6 +4063,8 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def,
/* create the serial port definition from the console definition */
if (def->nserials == 0) {
+virDomainChrConsoleTargetType type = def->consoles[0]->targetType;
+
if (VIR_APPEND_ELEMENT(def->serials,
def->nserials,
def->consoles[0]) < 0)
@@ -4034,7 +4072,9 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def,
/* modify it to be a serial port */
def->serials[0]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
-def->serials[0]->targetType =
VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE;
+virDomainChrConsoleTargetTypeToSerial(type,
+ &def->serials[0]->targetType,
+
&def->serials[0]->targetModel);
def->serials[0]->target.port = 0;
} else {
/* if the console source doesn't match */
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-s390-console2serial.args
b/tests/qemuxml2argvdata/qemuxml2argv-s390-console2serial.args
new file mode 100644
index 0..20968f794
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-s390-console2serial.args
@@ -0,0 +1,22 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-s390x \
+-name QEMUGuest1 \
+-S \
+
[libvirt] [PATCH v2 16/21] qemu: Support usb-serial and pci-serial on pSeries
The existing implementation set the address type for all serial
devices to spapr-vio, which made it impossible to use other devices
such as usb-serial and pci-serial; moreover, some decisions were
made based on the address type rather than the device type.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1512934
Signed-off-by: Andrea Bolognani
Reviewed-by: Pavel Hrdina
---
src/qemu/qemu_command.c| 9 --
src/qemu/qemu_domain_address.c | 3 +-
.../qemuxml2argv-pseries-serial-pci.args | 22 +++
.../qemuxml2argv-pseries-serial-pci.xml| 18
.../qemuxml2argv-pseries-serial-usb.args | 23
.../qemuxml2argv-pseries-serial-usb.xml| 21 ++
tests/qemuxml2argvtest.c | 7 +
.../qemuxml2xmlout-pseries-serial-pci.xml | 31 +
.../qemuxml2xmlout-pseries-serial-usb.xml | 32 ++
tests/qemuxml2xmltest.c| 7 +
10 files changed, 163 insertions(+), 10 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-pci.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-pci.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-usb.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-usb.xml
create mode 100644
tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-serial-pci.xml
create mode 100644
tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-serial-usb.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d1dd60d8f..96ff082d3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9150,15 +9150,6 @@ static bool
qemuChrIsPlatformDevice(const virDomainDef *def,
virDomainChrDefPtr chr)
{
-if ((def->os.arch == VIR_ARCH_PPC) || ARCH_IS_PPC64(def->os.arch)) {
-if (!qemuDomainIsPSeries(def))
-return true;
-/* only pseries need -device spapr-vty with -chardev */
-if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
-chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO)
-return true;
-}
-
if (def->os.arch == VIR_ARCH_ARMV7L || def->os.arch == VIR_ARCH_AARCH64) {
/* TARGET_TYPE_ISA here really means 'the default platform device' */
if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 2319e503e..f62bb2f97 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -246,8 +246,9 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
for (i = 0; i < def->nserials; i++) {
if (def->serials[i]->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
-qemuDomainIsPSeries(def))
+def->serials[i]->targetType ==
VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO) {
def->serials[i]->info.type =
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
+}
if (qemuDomainAssignSpaprVIOAddress(def, &def->serials[i]->info,
VIO_ADDR_SERIAL) < 0)
goto cleanup;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-pci.args
b/tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-pci.args
new file mode 100644
index 0..eb2a9bf0e
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-pci.args
@@ -0,0 +1,22 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-ppc64 \
+-name guest \
+-S \
+-M pseries \
+-m 512 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \
+-nographic \
+-nodefconfig \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-boot c \
+-chardev pty,id=charserial0 \
+-device pci-serial,chardev=charserial0,id=serial0,bus=pci.0,addr=0x1
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-pci.xml
b/tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-pci.xml
new file mode 100644
index 0..2c2534b4c
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-pci.xml
@@ -0,0 +1,18 @@
+
+ guest
+ 1ccfd97d-5eb4-478a-bbe6-88d254c16db7
+ 524288
+ 1
+
+hvm
+
+
+/usr/bin/qemu-system-ppc64
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-usb.args
b/tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-usb.args
new file mode 100644
index 0..0403985dc
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-usb.args
@@ -0,0 +1,23 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-ppc64 \
+-na
[libvirt] [PATCH v2 14/21] conf: Shorten names in virDomainChrSerialTarget enumeration
Now that the target type is no longer formatted on the QEMU command
line, we don't need the values to match the QEMU device names any
longer, so we can shorten the names and reduce redundancy by dropping
the -serial suffix: this also has the nice side-effect that target
type and address type will now match.
We still need to parse the old names, and format them when preparing
a migratable XML, to preserve backwards compatibility.
Signed-off-by: Andrea Bolognani
---
docs/formatdomain.html.in | 10 +++---
docs/schemas/domaincommon.rng | 4 +++
src/conf/domain_conf.c | 39 +++---
.../qemuargv2xml-console-compat.xml| 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-dev.xml | 2 +-
.../qemuargv2xmldata/qemuargv2xml-serial-file.xml | 2 +-
.../qemuargv2xmldata/qemuargv2xml-serial-many.xml | 4 +--
tests/qemuargv2xmldata/qemuargv2xml-serial-pty.xml | 2 +-
.../qemuargv2xml-serial-tcp-telnet.xml | 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-tcp.xml | 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-udp.xml | 4 +--
.../qemuargv2xmldata/qemuargv2xml-serial-unix.xml | 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-vc.xml | 2 +-
...otplug-console-compat-2-live+console-virtio.xml | 6 ++--
.../qemuhotplug-console-compat-2-live.xml | 6 ++--
...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 4 +--
.../qemuxml2argvdata/qemuxml2argv-user-aliases.xml | 4 +--
.../qemuxml2xmlout-aarch64-pci-serial.xml | 2 +-
.../qemuxml2xmlout-bios-nvram-os-interleave.xml| 2 +-
.../qemuxml2xmlout-chardev-label.xml | 4 +--
.../qemuxml2xmlout-console-compat-auto.xml | 2 +-
.../qemuxml2xmlout-console-compat.xml | 2 +-
.../qemuxml2xmlout-console-compat2.xml | 2 +-
.../qemuxml2xmlout-console-virtio-many.xml | 2 +-
.../qemuxml2xmlout-interface-driver.xml| 2 +-
.../qemuxml2xmlout-interface-server.xml| 4 +--
.../qemuxml2xmlout-net-bandwidth.xml | 2 +-
.../qemuxml2xmlout-net-bandwidth2.xml | 2 +-
.../qemuxml2xmlout-net-coalesce.xml| 2 +-
.../qemuxml2xmloutdata/qemuxml2xmlout-net-mtu.xml | 2 +-
.../qemuxml2xmlout-panic-pseries.xml | 2 +-
.../qemuxml2xmlout-pci-serial-dev-chardev.xml | 2 +-
.../qemuxml2xmlout-pseries-cpu-compat-power9.xml | 2 +-
.../qemuxml2xmlout-pseries-cpu-compat.xml | 2 +-
.../qemuxml2xmlout-pseries-cpu-exact.xml | 2 +-
.../qemuxml2xmlout-pseries-panic-missing.xml | 2 +-
.../qemuxml2xmlout-pseries-panic-no-address.xml| 2 +-
.../qemuxml2xmlout-q35-virt-manager-basic.xml | 2 +-
.../qemuxml2xmlout-serial-spiceport-nospice.xml| 2 +-
.../qemuxml2xmlout-serial-spiceport.xml| 2 +-
.../qemuxml2xmlout-serial-target-port-auto.xml | 6 ++--
.../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml | 4 +--
.../qemuxml2xmlout-tap-vhost-incorrect.xml | 2 +-
.../qemuxml2xmlout-tap-vhost.xml | 2 +-
.../qemuxml2xmlout-vhost_queues.xml| 2 +-
45 files changed, 99 insertions(+), 64 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 4bc88cfc5..2edc61a01 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6530,13 +6530,13 @@ qemu-kvm -net nic,model=? /dev/null
specifies the port number. Ports are numbered starting from 0. There are
usually 0, 1 or 2 serial ports. There is also an optional
type attribute since 1.0.2
- which has three choices for its value, one is isa-serial,
- then usb-serial and last one is pci-serial.
- If type is missing, isa-serial will be used by
- default. For usb-serial an optional sub-element
+ which has three choices for its value, one is isa,
+ then usb and last one is pci.
+ If type is missing, isa will be used by
+ default. For usb an optional sub-element
with type='usb' can tie the
device to a particular controller, documented
above.
- Similarly, pci-serial can be used to attach the device to
+ Similarly, pci can be used to attach the device to
the pci bus (since 1.2.16). Again, it has
optional sub-element with
type='pci' to select desired location on the PCI bus.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index fbba092d1..93beabc5e 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3582,6 +3582,10 @@
+isa
+usb
+pci
+
isa-serial
usb-serial
pci-serial
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 140f478b0..0d8c88db9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -449,9 +449,10 @@ VIR_ENUM_IMPL(virDomainChrD
[libvirt] [PATCH v2 15/21] conf: Add target type and model for spapr-vty
We can finally introduce a specific target model for the spapr-vty
device used by pSeries guests, which means isa-serial will no longer
show up to confuse users.
We make sure migration works in both directions by interpreting the
isa-serial target type, or the lack of target type, appropriately
when parsing the guest XML, and skipping the newly-introduced type
when formatting if for migration. We also verify that spapr-vty is
not used for non-pSeries guests and add a bunch of test cases.
This commit is best viewed with 'git diff -w'.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1511421
Signed-off-by: Andrea Bolognani
---
docs/formatdomain.html.in | 11 ++--
docs/schemas/domaincommon.rng | 2 +
src/conf/domain_conf.c | 4 ++
src/conf/domain_conf.h | 2 +
src/qemu/qemu_command.c| 74 ++
src/qemu/qemu_domain.c | 74 ++
src/qemu/qemu_domain_address.c | 1 +
.../qemuxml2argv-pseries-basic.args| 2 +-
.../qemuxml2argv-pseries-console-native.args | 1 +
.../qemuxml2argv-pseries-console-native.xml| 17 +
...gs => qemuxml2argv-pseries-console-virtio.args} | 10 +--
.../qemuxml2argv-pseries-console-virtio.xml| 19 ++
.../qemuxml2argv-pseries-cpu-compat-power9.args| 2 +-
.../qemuxml2argv-pseries-cpu-compat.args | 2 +-
.../qemuxml2argv-pseries-cpu-exact.args| 2 +-
.../qemuxml2argv-pseries-cpu-le.args | 2 +-
.../qemuxml2argv-pseries-panic-missing.args| 2 +-
.../qemuxml2argv-pseries-panic-no-address.args | 2 +-
...qemuxml2argv-pseries-serial+console-native.args | 1 +
.../qemuxml2argv-pseries-serial+console-native.xml | 18 ++
.../qemuxml2argv-pseries-serial-compat.args| 1 +
.../qemuxml2argv-pseries-serial-compat.xml | 19 ++
...qemuxml2argv-pseries-serial-invalid-machine.xml | 19 ++
...rgs => qemuxml2argv-pseries-serial-native.args} | 7 +-
.../qemuxml2argv-pseries-serial-native.xml | 16 +
.../qemuxml2argv-pseries-usb-default.args | 2 +-
.../qemuxml2argv-pseries-usb-kbd.args | 2 +-
.../qemuxml2argv-pseries-usb-multi.args| 2 +-
.../qemuxml2argv-pseries-vio-user-assigned.args| 4 +-
.../qemuxml2argvdata/qemuxml2argv-pseries-vio.args | 4 +-
tests/qemuxml2argvtest.c | 16 +
.../qemuxml2xmlout-panic-pseries.xml | 4 +-
.../qemuxml2xmlout-pseries-console-native.xml | 1 +
...l => qemuxml2xmlout-pseries-console-virtio.xml} | 18 ++
.../qemuxml2xmlout-pseries-cpu-compat-power9.xml | 4 +-
.../qemuxml2xmlout-pseries-cpu-compat.xml | 4 +-
.../qemuxml2xmlout-pseries-cpu-exact.xml | 4 +-
.../qemuxml2xmlout-pseries-panic-missing.xml | 4 +-
.../qemuxml2xmlout-pseries-panic-no-address.xml| 4 +-
...emuxml2xmlout-pseries-serial+console-native.xml | 1 +
.../qemuxml2xmlout-pseries-serial-compat.xml | 1 +
...ml => qemuxml2xmlout-pseries-serial-native.xml} | 10 ++-
tests/qemuxml2xmltest.c| 15 +
43 files changed, 301 insertions(+), 109 deletions(-)
create mode 12
tests/qemuxml2argvdata/qemuxml2argv-pseries-console-native.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-pseries-console-native.xml
copy tests/qemuxml2argvdata/{qemuxml2argv-pseries-basic.args =>
qemuxml2argv-pseries-console-virtio.args} (59%)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-pseries-console-virtio.xml
create mode 12
tests/qemuxml2argvdata/qemuxml2argv-pseries-serial+console-native.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-pseries-serial+console-native.xml
create mode 12
tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-compat.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-compat.xml
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-invalid-machine.xml
copy tests/qemuxml2argvdata/{qemuxml2argv-pseries-basic.args =>
qemuxml2argv-pseries-serial-native.args} (70%)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-native.xml
create mode 12
tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-console-native.xml
copy tests/qemuxml2xmloutdata/{qemuxml2xmlout-panic-pseries.xml =>
qemuxml2xmlout-pseries-console-virtio.xml} (71%)
create mode 12
tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-serial+console-native.xml
create mode 12
tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-serial-compat.xml
copy tests/qemuxml2xmloutdata/{qemuxml2xmlout-panic-pseries.xml =>
qemuxml2xmlout-pseries-serial-native.xml} (79%)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 2edc61a01..92622d031 100644
--- a/docs/formatdomain.html.in
+++ b/docs
[libvirt] [PATCH v2 17/21] conf: Add target type and model for pl011
We can finally introduce a specific target model for the pl011 device used by mach-virt guests, which means isa-serial will no longer show up to confuse users. We make sure migration works in both directions by interpreting the isa-serial target type, or the lack of target type, appropriately when parsing the guest XML, and skipping the newly-introduced type when formatting if for migration. We also verify that pl011 is not used for non-mach-virt guests and add a bunch of test cases. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=151292 Signed-off-by: Andrea Bolognani --- docs/formatdomain.html.in | 2 +- docs/schemas/domaincommon.rng | 2 + src/conf/domain_conf.c | 4 ++ src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c| 8 +++- src/qemu/qemu_domain.c | 37 ++ src/qemu/qemu_domain_address.c | 1 + .../qemuxml2argv-mach-virt-console-native.args | 1 + .../qemuxml2argv-mach-virt-console-native.xml | 17 + .../qemuxml2argv-mach-virt-console-virtio.args | 24 .../qemuxml2argv-mach-virt-console-virtio.xml | 19 ++ ...muxml2argv-mach-virt-serial+console-native.args | 1 + ...emuxml2argv-mach-virt-serial+console-native.xml | 18 + .../qemuxml2argv-mach-virt-serial-compat.args | 1 + .../qemuxml2argv-mach-virt-serial-compat.xml | 19 ++ ...muxml2argv-mach-virt-serial-invalid-machine.xml | 21 +++ .../qemuxml2argv-mach-virt-serial-native.args | 23 +++ .../qemuxml2argv-mach-virt-serial-native.xml | 16 .../qemuxml2argv-mach-virt-serial-pci.args | 26 + .../qemuxml2argv-mach-virt-serial-pci.xml | 18 + .../qemuxml2argv-mach-virt-serial-usb.args | 27 + .../qemuxml2argv-mach-virt-serial-usb.xml | 21 +++ tests/qemuxml2argvtest.c | 27 + .../qemuxml2xmlout-aarch64-virtio-pci-default.xml | 4 +- .../qemuxml2xmlout-mach-virt-console-native.xml| 1 + .../qemuxml2xmlout-mach-virt-console-virtio.xml| 27 + ...uxml2xmlout-mach-virt-serial+console-native.xml | 1 + .../qemuxml2xmlout-mach-virt-serial-compat.xml | 31 +++ .../qemuxml2xmlout-mach-virt-serial-native.xml | 1 + .../qemuxml2xmlout-mach-virt-serial-pci.xml| 44 ++ .../qemuxml2xmlout-mach-virt-serial-usb.xml| 41 tests/qemuxml2xmltest.c| 26 + 32 files changed, 507 insertions(+), 4 deletions(-) create mode 12 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-console-native.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-console-native.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-console-virtio.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-console-virtio.xml create mode 12 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial+console-native.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial+console-native.xml create mode 12 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial-compat.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial-compat.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial-invalid-machine.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial-native.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial-native.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial-pci.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial-pci.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial-usb.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-mach-virt-serial-usb.xml create mode 12 tests/qemuxml2xmloutdata/qemuxml2xmlout-mach-virt-console-native.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-mach-virt-console-virtio.xml create mode 12 tests/qemuxml2xmloutdata/qemuxml2xmlout-mach-virt-serial+console-native.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-mach-virt-serial-compat.xml create mode 12 tests/qemuxml2xmloutdata/qemuxml2xmlout-mach-virt-serial-native.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-mach-virt-serial-pci.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-mach-virt-serial-usb.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 92622d031..1efded6be 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6532,7 +6532,7 @@ qemu-kvm -net nic,model=? /dev/null type attribute since 1.0.2 which can be used to pick between isa, usb, p
[libvirt] [PATCH v2 12/21] qemu: Validate target model for serial devices
Target model and target type must agree for the configuration
to make sense, so check that's actually the case and error out
otherwise.
Signed-off-by: Andrea Bolognani
---
src/libvirt_private.syms | 2 ++
src/qemu/qemu_domain.c | 37 +
2 files changed, 39 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d3ca6b2ec..0fb7d0e81 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -209,6 +209,8 @@ virDomainChrGetDomainPtrs;
virDomainChrInsertPreAlloced;
virDomainChrPreAlloc;
virDomainChrRemove;
+virDomainChrSerialTargetModelTypeFromString;
+virDomainChrSerialTargetModelTypeToString;
virDomainChrSerialTargetTypeFromString;
virDomainChrSerialTargetTypeToString;
virDomainChrSourceDefClear;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 32cb62fb9..06ce382fa 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3538,6 +3538,43 @@ qemuDomainChrTargetDefValidate(const virDomainDef *def,
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
break;
}
+
+/* Validate target model */
+switch ((virDomainChrSerialTargetModel) chr->targetModel) {
+case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL:
+if (chr->targetType != VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target model '%s' requires "
+ "target type 'isa'"),
+
virDomainChrSerialTargetModelTypeToString(chr->targetModel));
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL:
+if (chr->targetType != VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target model '%s' requires "
+ "target type 'usb'"),
+
virDomainChrSerialTargetModelTypeToString(chr->targetModel));
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL:
+if (chr->targetType != VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target model '%s' requires "
+ "target type 'pci'"),
+
virDomainChrSerialTargetModelTypeToString(chr->targetModel));
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST:
+break;
+}
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 11/21] qemu: Set targetModel based on targetType for serial devices
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_domain.c | 20
.../qemuargv2xmldata/qemuargv2xml-console-compat.xml | 4 +++-
tests/qemuargv2xmldata/qemuargv2xml-serial-dev.xml | 4 +++-
tests/qemuargv2xmldata/qemuargv2xml-serial-file.xml | 4 +++-
tests/qemuargv2xmldata/qemuargv2xml-serial-many.xml | 8 ++--
tests/qemuargv2xmldata/qemuargv2xml-serial-pty.xml | 4 +++-
.../qemuargv2xml-serial-tcp-telnet.xml | 4 +++-
tests/qemuargv2xmldata/qemuargv2xml-serial-tcp.xml | 4 +++-
tests/qemuargv2xmldata/qemuargv2xml-serial-udp.xml | 8 ++--
tests/qemuargv2xmldata/qemuargv2xml-serial-unix.xml | 4 +++-
tests/qemuargv2xmldata/qemuargv2xml-serial-vc.xml| 4 +++-
...uhotplug-console-compat-2-live+console-virtio.xml | 12 +---
.../qemuhotplug-console-compat-2-live.xml| 12 +---
...qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 8 ++--
tests/qemuxml2argvdata/qemuxml2argv-user-aliases.xml | 8 ++--
.../qemuxml2xmlout-aarch64-pci-serial.xml| 4 +++-
.../qemuxml2xmlout-bios-nvram-os-interleave.xml | 4 +++-
.../qemuxml2xmlout-chardev-label.xml | 8 ++--
.../qemuxml2xmlout-console-compat-auto.xml | 4 +++-
.../qemuxml2xmlout-console-compat.xml| 4 +++-
.../qemuxml2xmlout-console-compat2.xml | 4 +++-
.../qemuxml2xmlout-console-virtio-many.xml | 4 +++-
.../qemuxml2xmlout-interface-driver.xml | 4 +++-
.../qemuxml2xmlout-interface-server.xml | 8 ++--
.../qemuxml2xmlout-net-bandwidth.xml | 4 +++-
.../qemuxml2xmlout-net-bandwidth2.xml| 4 +++-
.../qemuxml2xmlout-net-coalesce.xml | 4 +++-
tests/qemuxml2xmloutdata/qemuxml2xmlout-net-mtu.xml | 4 +++-
.../qemuxml2xmlout-panic-pseries.xml | 4 +++-
.../qemuxml2xmlout-pci-serial-dev-chardev.xml| 4 +++-
.../qemuxml2xmlout-pseries-cpu-compat-power9.xml | 4 +++-
.../qemuxml2xmlout-pseries-cpu-compat.xml| 4 +++-
.../qemuxml2xmlout-pseries-cpu-exact.xml | 4 +++-
.../qemuxml2xmlout-pseries-panic-missing.xml | 4 +++-
.../qemuxml2xmlout-pseries-panic-no-address.xml | 4 +++-
.../qemuxml2xmlout-q35-virt-manager-basic.xml| 4 +++-
.../qemuxml2xmlout-serial-spiceport-nospice.xml | 4 +++-
.../qemuxml2xmlout-serial-spiceport.xml | 4 +++-
.../qemuxml2xmlout-serial-target-port-auto.xml | 12 +---
.../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml| 8 ++--
.../qemuxml2xmlout-tap-vhost-incorrect.xml | 4 +++-
.../qemuxml2xmloutdata/qemuxml2xmlout-tap-vhost.xml | 4 +++-
.../qemuxml2xmlout-vhost_queues.xml | 4 +++-
43 files changed, 185 insertions(+), 55 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 12b2a0bf6..32cb62fb9 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4135,6 +4135,26 @@ qemuDomainChrDefPostParse(virDomainChrDefPtr chr,
}
}
+/* Set the default target model */
+if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+chr->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE) {
+switch ((virDomainChrSerialTargetType) chr->targetType) {
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
+chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL;
+break;
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
+chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL;
+break;
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
+chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL;
+break;
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
+/* Nothing to do */
+break;
+}
+}
+
/* clear auto generated unix socket path for inactive definitions */
if (parseFlags & VIR_DOMAIN_DEF_PARSE_INACTIVE) {
if (qemuDomainChrDefDropDefaultPath(chr, driver) < 0)
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-console-compat.xml
b/tests/qemuargv2xmldata/qemuargv2xml-console-compat.xml
index 7c106f145..cba43ca45 100644
--- a/tests/qemuargv2xmldata/qemuargv2xml-console-compat.xml
+++ b/tests/qemuargv2xmldata/qemuargv2xml-console-compat.xml
@@ -28,7 +28,9 @@
-
+
+
+
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-serial-dev.xml
b/tests/qemuargv2xmldata/qemuargv2xml-serial-dev.xml
index e76d0211d..e9998d554 100644
--- a/tests/qemuargv2xmldata/qemuargv2xml-serial-dev.xml
+++ b/tests/qemuargv2xmldata/qemuargv2xml-serial-dev.xml
@@ -29,7 +29,9 @@
-
+
+
+
diff --git a/tests/qemuargv2xmldata/
[libvirt] [PATCH v2 13/21] qemu: Format targetModel for serial devices
Now that we've created a distinction between target type and target
model, with the latter being the concrete device name, it's time to
switch to formatting the model instead of the type.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_command.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 86521d498..d49183931 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10282,8 +10282,8 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
serial->info.alias);
}
} else {
-switch ((virDomainChrSerialTargetType) serial->targetType) {
-case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
+switch ((virDomainChrSerialTargetModel) serial->targetModel) {
+case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_SERIAL)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("usb-serial is not supported in this QEMU
binary"));
@@ -10291,10 +10291,10 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
}
break;
-case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL:
break;
-case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_SERIAL)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("pci-serial is not supported with this QEMU
binary"));
@@ -10302,8 +10302,8 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
}
break;
-case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
-case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST:
/* Except from _LAST, which is just a guard value and will never
* be used, all of the above are platform devices, which means
* qemuBuildSerialCommandLine() will have taken the appropriate
@@ -10314,7 +10314,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
}
virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
-
virDomainChrSerialTargetTypeToString(serial->targetType),
+
virDomainChrSerialTargetModelTypeToString(serial->targetModel),
serial->info.alias, serial->info.alias);
}
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 08/21] conf: Remove ATTRIBUTE_FALLTHROUGH from virDomainChrTargetDefFormat()
Formatting the element for serial devices will become a
bit more complicated later on, and leaving the fallthrough behavior
there would do nothing but complicate it further.
Signed-off-by: Andrea Bolognani
---
src/conf/domain_conf.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index cd9d384d3..9d6e06025 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -24007,14 +24007,18 @@ virDomainChrTargetDefFormat(virBufferPtr buf,
return -1;
}
+virBufferAddLit(buf, "targetType != VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE) {
virBufferAsprintf(buf,
- "\n",
- targetType,
- def->target.port);
-break;
+ "type='%s' ",
+ targetType);
}
-ATTRIBUTE_FALLTHROUGH;
+
+virBufferAsprintf(buf,
+ "port='%d'/>\n",
+ def->target.port);
+break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
virBufferAsprintf(buf, "\n",
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 10/21] conf: Parse and format virDomainChrSerialTargetModel
This information will be used to select, and store in the guest
configuration in order to guarantee ABI stability, the concrete
(hypervisor-specific) model for serial devices.
Signed-off-by: Andrea Bolognani
---
docs/schemas/domaincommon.rng | 15 +
src/conf/domain_conf.c| 72 ++-
src/conf/domain_conf.h| 12
3 files changed, 98 insertions(+), 1 deletion(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f1808065b..fbba092d1 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3589,6 +3589,18 @@
+
+
+
+
+ isa-serial
+ usb-serial
+ pci-serial
+
+
+
+
+
@@ -3603,6 +3615,9 @@
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9d6e06025..140f478b0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -472,6 +472,14 @@ VIR_ENUM_IMPL(virDomainChrConsoleTarget,
"sclp",
"sclplm")
+VIR_ENUM_IMPL(virDomainChrSerialTargetModel,
+ VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST,
+ "none",
+ "isa-serial",
+ "usb-serial",
+ "pci-serial",
+);
+
VIR_ENUM_IMPL(virDomainChrDevice, VIR_DOMAIN_CHR_DEVICE_TYPE_LAST,
"parallel",
"serial",
@@ -11526,14 +11534,42 @@ virDomainChrTargetTypeFromString(int devtype,
return ret;
}
+static int
+virDomainChrTargetModelFromString(int devtype,
+ const char *targetModel)
+{
+int ret = -1;
+
+if (!targetModel)
+return 0;
+
+switch ((virDomainChrDeviceType) devtype) {
+case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+ret = virDomainChrSerialTargetModelTypeFromString(targetModel);
+break;
+
+case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
+case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
+case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
+case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST:
+/* Target model not supported yet */
+ret = 0;
+break;
+}
+
+return ret;
+}
+
static int
virDomainChrDefParseTargetXML(virDomainChrDefPtr def,
xmlNodePtr cur,
unsigned int flags)
{
int ret = -1;
+xmlNodePtr child;
unsigned int port;
char *targetType = virXMLPropString(cur, "type");
+char *targetModel = NULL;
char *addrStr = NULL;
char *portStr = NULL;
char *stateStr = NULL;
@@ -11547,6 +11583,24 @@ virDomainChrDefParseTargetXML(virDomainChrDefPtr def,
goto error;
}
+child = cur->children;
+while (child != NULL) {
+if (child->type == XML_ELEMENT_NODE &&
+virXMLNodeNameEqual(child, "model")) {
+targetModel = virXMLPropString(child, "name");
+}
+child = child->next;
+}
+
+if ((def->targetModel =
+ virDomainChrTargetModelFromString(def->deviceType,
+ targetModel)) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown target model '%s' specified for character
device"),
+ targetModel);
+goto error;
+}
+
switch (def->deviceType) {
case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
switch (def->targetType) {
@@ -11635,6 +11689,7 @@ virDomainChrDefParseTargetXML(virDomainChrDefPtr def,
ret = 0;
error:
VIR_FREE(targetType);
+VIR_FREE(targetModel);
VIR_FREE(addrStr);
VIR_FREE(portStr);
VIR_FREE(stateStr);
@@ -24016,8 +24071,23 @@ virDomainChrTargetDefFormat(virBufferPtr buf,
}
virBufferAsprintf(buf,
- "port='%d'/>\n",
+ "port='%d'",
def->target.port);
+
+if (def->targetModel != VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE) {
+virBufferAddLit(buf, ">\n");
+
+virBufferAdjustIndent(buf, 2);
+virBufferAsprintf(buf,
+ "\n",
+
virDomainChrSerialTargetModelTypeToString(def->targetModel));
+virBufferAdjustIndent(buf, -2);
+
+virBufferAddLit(buf, "\n");
+} else {
+virBufferAddLit(buf, "/>\n");
+}
+
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 5d4d17ed6..3e74c635b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1112,6 +1112,17 @@ typedef enum {
VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST
} virDomainChrConsoleTargetType;
+typedef enum {
+VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE = 0,
+VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL,
+VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL,
+V
[libvirt] [PATCH v2 09/21] qemu: Introduce qemuDomainChrTargetDefValidate()
Instead of waiting until we get to command line generation, we can
validate the target for a char device much earlier.
Move all the checks out of qemuBuildSerialChrDeviceStr() and into
the new fuction. This will later allow us to validate the target
for platform devices.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_command.c | 20 ---
src/qemu/qemu_domain.c | 65 +
2 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d534bc8ad..86521d498 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10289,22 +10289,9 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("usb-serial is not supported in this QEMU
binary"));
goto error;
}
-
-if (serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
-serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("usb-serial requires address of usb type"));
-goto error;
-}
break;
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
-if (serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
-serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("isa-serial requires address of isa type"));
-goto error;
-}
break;
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
@@ -10313,13 +10300,6 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("pci-serial is not supported with this QEMU
binary"));
goto error;
}
-
-if (serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
-serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("pci-serial requires address of pci type"));
-goto error;
-}
break;
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b60c374d9..12b2a0bf6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3490,6 +3490,68 @@ qemuDomainChrSourceDefValidate(const
virDomainChrSourceDef *def)
}
+static int
+qemuDomainChrTargetDefValidate(const virDomainDef *def,
+ const virDomainChrDef *chr)
+{
+switch ((virDomainChrDeviceType) chr->deviceType) {
+case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+
+/* Validate target type */
+switch ((virDomainChrSerialTargetType) chr->targetType) {
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
+/* Hack required until we have a proper type for pSeries
+ * serial consoles */
+if (qemuDomainIsPSeries(def))
+return 0;
+
+if (chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Target type 'isa-serial' requires address "
+ "of type 'isa'"));
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
+if (chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Target type 'usb-serial' requires address "
+ "of type 'usb'"));
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
+if (chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Target type 'pci-serial' requires address "
+ "of type 'pci'"));
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
+break;
+}
+break;
+
+case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
+case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
+case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
+case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST:
+/* Nothing to do */
+break;
+}
+
+return 0;
+}
+
+
static int
qemuDomainChrDefValidate(const virDomainChrDef *dev,
const virDomainDef *def)
@@
[libvirt] [PATCH v2 07/21] conf: Improve virDomainChrTargetDefFormat()
Make the switch statement type-aware, avoid calling
virDomainChrTargetTypeToString() more than once and check its
return value before using it.
Signed-off-by: Andrea Bolognani
---
src/conf/domain_conf.c | 29 ++---
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index cb4ed6ef6..cd9d384d3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -23942,7 +23942,7 @@ virDomainChrTargetDefFormat(virBufferPtr buf,
const char *targetType = virDomainChrTargetTypeToString(def->deviceType,
def->targetType);
-switch (def->deviceType) {
+switch ((virDomainChrDeviceType) def->deviceType) {
case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: {
if (!targetType) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -23989,28 +23989,43 @@ virDomainChrTargetDefFormat(virBufferPtr buf,
}
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
+if (!targetType) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not format console target type"));
+return -1;
+}
+
virBufferAsprintf(buf,
"\n",
- virDomainChrTargetTypeToString(def->deviceType,
- def->targetType),
- def->target.port);
+ targetType, def->target.port);
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+if (!targetType) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not format serial target type"));
+return -1;
+}
+
if (def->targetType != VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE) {
virBufferAsprintf(buf,
"\n",
- virDomainChrTargetTypeToString(def->deviceType,
- def->targetType),
+ targetType,
def->target.port);
break;
}
ATTRIBUTE_FALLTHROUGH;
-default:
+case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
virBufferAsprintf(buf, "\n",
def->target.port);
break;
+
+case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST:
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected char device type %d"),
+ def->deviceType);
+return -1;
}
return 0;
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 03/21] conf: Introduce VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE
This is the first step in getting rid of the assumption that
isa-serial is the default target type for serial devices.
Signed-off-by: Andrea Bolognani
Reviewed-by: Pavel Hrdina
---
src/conf/domain_conf.c | 8 +---
src/conf/domain_conf.h | 3 ++-
src/qemu/qemu_command.c| 13 +
src/qemu/qemu_domain.c | 21 +
src/qemu/qemu_domain_address.c | 1 +
5 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 20a4acd74..321ee6a0f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -448,6 +448,7 @@ VIR_ENUM_IMPL(virDomainChrDeviceState,
VIR_DOMAIN_CHR_DEVICE_STATE_LAST,
VIR_ENUM_IMPL(virDomainChrSerialTarget,
VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST,
+ "none",
"isa-serial",
"usb-serial",
"pci-serial")
@@ -4016,7 +4017,7 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def)
/* modify it to be a serial port */
def->serials[0]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
-def->serials[0]->targetType =
VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA;
+def->serials[0]->targetType =
VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE;
def->serials[0]->target.port = 0;
} else {
/* if the console source doesn't match */
@@ -4040,7 +4041,8 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def)
def->serials[0]->deviceType ==
VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL) {
switch ((virDomainChrSerialTargetType) def->serials[0]->targetType) {
-case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA: {
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: {
/* Create a stub console to match the serial port.
* console[0] either does not exist
@@ -11481,7 +11483,7 @@ virDomainChrDefaultTargetType(int devtype)
return VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE;
case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
-return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA;
+return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE;
case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index cb8701dd2..9b88bf19d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1081,7 +1081,8 @@ typedef enum {
} virDomainChrDeviceType;
typedef enum {
-VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA = 0,
+VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE = 0,
+VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA,
VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB,
VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 216a4bdfe..d534bc8ad 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9166,6 +9166,14 @@ qemuChrIsPlatformDevice(const virDomainDef *def,
return true;
}
+/* If we got all the way here and we're still stuck with the default
+ * target type for a serial device, it means we have no clue what kind of
+ * device we're talking about and we must treat it as a platform device. */
+if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE) {
+return true;
+}
+
return false;
}
@@ -10314,7 +10322,12 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
}
break;
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
+/* Except from _LAST, which is just a guard value and will never
+ * be used, all of the above are platform devices, which means
+ * qemuBuildSerialCommandLine() will have taken the appropriate
+ * branch and we will not have ended up here. */
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Invalid target type for serial device"));
goto error;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c6ba4079d..b60c374d9 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4049,6 +4049,27 @@ qemuDomainChrDefPostParse(virDomainChrDefPtr chr,
chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
}
+/* Historically, isa-serial and the default matched, so in order to
+ * maintain backwards compatibility we map them here. The actual default
+ * will be picked below based on the architecture and machine type. */
+if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA) {
+chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE;
+}
+
+/* Set the default serial type */
+if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+chr->targetType ==
[libvirt] [PATCH v2 06/21] conf: Improve error handling in virDomainChrDefFormat()
We don't need to store the return value since we never modify it; we
should also report failure when virDomainChrSourceDefFormat() fails.
Signed-off-by: Andrea Bolognani
---
src/conf/domain_conf.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 224b88d9a..cb4ed6ef6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -24025,8 +24025,6 @@ virDomainChrDefFormat(virBufferPtr buf,
const char *elementName = virDomainChrDeviceTypeToString(def->deviceType);
bool tty_compat;
-int ret = 0;
-
if (!elementName) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected char device type %d"),
@@ -24044,7 +24042,9 @@ virDomainChrDefFormat(virBufferPtr buf,
if (virDomainChrAttrsDefFormat(buf, def->source, tty_compat) < 0)
return -1;
virBufferAddLit(buf, ">\n");
-virDomainChrSourceDefFormat(buf, def->source, flags);
+
+if (virDomainChrSourceDefFormat(buf, def->source, flags) < 0)
+ return -1;
if (virDomainChrTargetDefFormat(buf, def, flags) < 0)
return -1;
@@ -24054,7 +24054,7 @@ virDomainChrDefFormat(virBufferPtr buf,
virBufferAdjustIndent(buf, -2);
virBufferAsprintf(buf, "\n", elementName);
-return ret;
+return 0;
}
static int
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 01/21] qemu: Introduce qemuDomainChrDefPostParse()
Having a separate function for char device handling is better than
adding even more code to qemuDomainDeviceDefPostParse().
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_domain.c | 57 ++
1 file changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cc7596bad..c6ba4079d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4036,6 +4036,35 @@
qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
return 0;
}
+static int
+qemuDomainChrDefPostParse(virDomainChrDefPtr chr,
+ const virDomainDef *def,
+ virQEMUDriverPtr driver,
+ unsigned int parseFlags)
+{
+/* set the default console type for S390 arches */
+if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
+chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE &&
+ARCH_IS_S390(def->os.arch)) {
+chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
+}
+
+/* clear auto generated unix socket path for inactive definitions */
+if (parseFlags & VIR_DOMAIN_DEF_PARSE_INACTIVE) {
+if (qemuDomainChrDefDropDefaultPath(chr, driver) < 0)
+return -1;
+
+/* For UNIX chardev if no path is provided we generate one.
+ * This also implies that the mode is 'bind'. */
+if (chr->source &&
+chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
+!chr->source->data.nix.path) {
+chr->source->data.nix.listen = true;
+}
+}
+
+return 0;
+}
static int
qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
@@ -4096,29 +4125,6 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
}
}
-/* set the default console type for S390 arches */
-if (dev->type == VIR_DOMAIN_DEVICE_CHR &&
-dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
-dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE &&
-ARCH_IS_S390(def->os.arch))
-dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
-
-/* clear auto generated unix socket path for inactive definitions */
-if ((parseFlags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
-dev->type == VIR_DOMAIN_DEVICE_CHR) {
-virDomainChrDefPtr chr = dev->data.chr;
-if (qemuDomainChrDefDropDefaultPath(chr, driver) < 0)
-goto cleanup;
-
-/* For UNIX chardev if no path is provided we generate one.
- * This also implies that the mode is 'bind'. */
-if (chr->source &&
-chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
-!chr->source->data.nix.path) {
-chr->source->data.nix.listen = true;
-}
-}
-
if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) {
if (dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
if ARCH_IS_PPC64(def->os.arch)
@@ -4154,6 +4160,11 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
qemuDomainShmemDefPostParse(dev->data.shmem) < 0)
goto cleanup;
+if (dev->type == VIR_DOMAIN_DEVICE_CHR &&
+qemuDomainChrDefPostParse(dev->data.chr, def, driver, parseFlags) < 0)
{
+goto cleanup;
+}
+
ret = 0;
cleanup:
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 04/21] conf: Drop virDomainChrDeviceType.targetTypeAttr
This attribute was used to decide whether to format the type
attribute of the element, but the logic didn't take into
account all possible cases and as such could lead to unexpected
results. Moreover, it's one more thing to keep track of, and can
easily fall out of sync with other attributes.
Now that we have VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE, we can
use that value to signal that no specific target type has been
configured for the serial device and as such the attribute should
not be formatted at all. All other values are now formatted.
Signed-off-by: Andrea Bolognani
Reviewed-by: Pavel Hrdina
---
src/conf/domain_conf.c| 11 ---
src/conf/domain_conf.h| 1 -
src/vz/vz_sdk.c | 3 +--
tests/qemuargv2xmldata/qemuargv2xml-console-compat.xml| 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-dev.xml| 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-file.xml | 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-many.xml | 4 ++--
tests/qemuargv2xmldata/qemuargv2xml-serial-pty.xml| 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-tcp-telnet.xml | 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-tcp.xml| 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-udp.xml| 4 ++--
tests/qemuargv2xmldata/qemuargv2xml-serial-unix.xml | 2 +-
tests/qemuargv2xmldata/qemuargv2xml-serial-vc.xml | 2 +-
.../qemuhotplug-console-compat-2-live+console-virtio.xml | 4 ++--
.../qemuhotplug-console-compat-2-live.xml | 4 ++--
.../qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 4 ++--
tests/qemuxml2argvdata/qemuxml2argv-user-aliases.xml | 4 ++--
.../qemuxml2xmlout-bios-nvram-os-interleave.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-chardev-label.xml | 4 ++--
.../qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat.xml| 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat2.xml | 2 +-
.../qemuxml2xmloutdata/qemuxml2xmlout-console-virtio-many.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-interface-driver.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-interface-server.xml | 4 ++--
tests/qemuxml2xmloutdata/qemuxml2xmlout-net-bandwidth.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-net-bandwidth2.xml| 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-net-coalesce.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-net-mtu.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml | 2 +-
.../qemuxml2xmlout-pseries-cpu-compat-power9.xml | 2 +-
.../qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-exact.xml | 2 +-
.../qemuxml2xmlout-pseries-panic-missing.xml | 2 +-
.../qemuxml2xmlout-pseries-panic-no-address.xml | 2 +-
.../qemuxml2xmlout-q35-virt-manager-basic.xml | 2 +-
.../qemuxml2xmlout-serial-spiceport-nospice.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-spiceport.xml | 2 +-
.../qemuxml2xmlout-serial-target-port-auto.xml| 6 +++---
.../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml | 4 ++--
.../qemuxml2xmloutdata/qemuxml2xmlout-tap-vhost-incorrect.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-tap-vhost.xml | 2 +-
tests/qemuxml2xmloutdata/qemuxml2xmlout-vhost_queues.xml | 2 +-
43 files changed, 56 insertions(+), 61 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 321ee6a0f..e4c537136 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11495,8 +11495,7 @@ virDomainChrDefaultTargetType(int devtype)
}
static int
-virDomainChrTargetTypeFromString(virDomainChrDefPtr def,
- int devtype,
+virDomainChrTargetTypeFromString(int devtype,
const char *targetType)
{
int ret = -1;
@@ -11524,8 +11523,6 @@ virDomainChrTargetTypeFromString(virDomainChrDefPtr def,
break;
}
-def->targetTypeAttr = true;
-
return ret;
}
@@ -11542,7 +11539,7 @@ virDomainChrDefParseTargetXML(virDomainChrDefPtr def,
char *stateStr = NULL;
if ((def->targetType =
- virDomainChrTargetTypeFromString(def, def->deviceType,
+ virDomainChrTargetTypeFromString(def->deviceType,
targetType)) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown target type '%s' specified for character
device"),
@@ -16462,7 +16459,7 @@ virDomainChrEquals(virDomainChrDefPtr src,
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
-if (src->tar
[libvirt] [PATCH v2 00/21] Fix serial console behavior on non-x86 architectures
Changes from [v1]: * introduce target model to go along with target type and store the actual hypervisor-specific device name, as suggested by Pavel; * shorten the names of the esisting target types; * introduce a bunch of additional cleanups required for the first item. I've kept existing R-bs because the changes didn't IMHO invalidate them, but feel free to double check and possibly disagree. Proper documentation will come as a follow-up, see the previous version to get an idea of what it will look like. [1] https://www.redhat.com/archives/libvir-list/2017-November/msg00545.html Andrea Bolognani (17): qemu: Introduce qemuDomainChrDefPostParse() conf: Run devicePostParse() again for the first serial device conf: Introduce VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE conf: Drop virDomainChrDeviceType.targetTypeAttr conf: Introduce virDomainChrTargetDefFormat() conf: Improve error handling in virDomainChrDefFormat() conf: Improve virDomainChrTargetDefFormat() conf: Remove ATTRIBUTE_FALLTHROUGH from virDomainChrTargetDefFormat() qemu: Introduce qemuDomainChrTargetDefValidate() conf: Parse and format virDomainChrSerialTargetModel qemu: Set targetModel based on targetType for serial devices qemu: Validate target model for serial devices qemu: Format targetModel for serial devices conf: Shorten names in virDomainChrSerialTarget enumeration conf: Add target type and model for spapr-vty qemu: Support usb-serial and pci-serial on pSeries conf: Add target type and model for pl011 Pino Toscano (4): conf: add VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP conf: pass parseFlags down to virDomainDefAddConsoleCompat conf: convert sclp/sclplm as qemu: switch s390/s390x default console back to serial docs/formatdomain.html.in | 13 +- docs/schemas/domaincommon.rng | 26 ++ src/conf/domain_conf.c | 311 + src/conf/domain_conf.h | 26 +- src/libvirt_private.syms | 2 + src/qemu/qemu_command.c| 126 - src/qemu/qemu_domain.c | 310 ++-- src/qemu/qemu_domain_address.c | 7 +- src/vz/vz_sdk.c| 5 +- .../qemuargv2xml-console-compat.xml| 4 +- tests/qemuargv2xmldata/qemuargv2xml-serial-dev.xml | 4 +- .../qemuargv2xmldata/qemuargv2xml-serial-file.xml | 4 +- .../qemuargv2xmldata/qemuargv2xml-serial-many.xml | 8 +- tests/qemuargv2xmldata/qemuargv2xml-serial-pty.xml | 4 +- .../qemuargv2xml-serial-tcp-telnet.xml | 4 +- tests/qemuargv2xmldata/qemuargv2xml-serial-tcp.xml | 4 +- tests/qemuargv2xmldata/qemuargv2xml-serial-udp.xml | 8 +- .../qemuargv2xmldata/qemuargv2xml-serial-unix.xml | 4 +- tests/qemuargv2xmldata/qemuargv2xml-serial-vc.xml | 4 +- ...otplug-console-compat-2-live+console-virtio.xml | 12 +- .../qemuhotplug-console-compat-2-live.xml | 12 +- .../qemuxml2argv-mach-virt-console-native.args | 1 + .../qemuxml2argv-mach-virt-console-native.xml | 17 ++ ... => qemuxml2argv-mach-virt-console-virtio.args} | 15 +- .../qemuxml2argv-mach-virt-console-virtio.xml | 19 ++ ...muxml2argv-mach-virt-serial+console-native.args | 1 + ...emuxml2argv-mach-virt-serial+console-native.xml | 18 ++ .../qemuxml2argv-mach-virt-serial-compat.args | 1 + .../qemuxml2argv-mach-virt-serial-compat.xml | 19 ++ ...muxml2argv-mach-virt-serial-invalid-machine.xml | 21 ++ ...s => qemuxml2argv-mach-virt-serial-native.args} | 12 +- .../qemuxml2argv-mach-virt-serial-native.xml | 16 ++ .../qemuxml2argv-mach-virt-serial-pci.args | 26 ++ .../qemuxml2argv-mach-virt-serial-pci.xml | 18 ++ .../qemuxml2argv-mach-virt-serial-usb.args | 27 ++ .../qemuxml2argv-mach-virt-serial-usb.xml | 21 ++ .../qemuxml2argv-pseries-basic.args| 2 +- .../qemuxml2argv-pseries-console-native.args | 1 + .../qemuxml2argv-pseries-console-native.xml| 17 ++ ...gs => qemuxml2argv-pseries-console-virtio.args} | 10 +- .../qemuxml2argv-pseries-console-virtio.xml| 19 ++ .../qemuxml2argv-pseries-cpu-compat-power9.args| 2 +- .../qemuxml2argv-pseries-cpu-compat.args | 2 +- .../qemuxml2argv-pseries-cpu-exact.args| 2 +- .../qemuxml2argv-pseries-cpu-le.args | 2 +- .../qemuxml2argv-pseries-panic-missing.args| 2 +- .../qemuxml2argv-pseries-panic-no-address.args | 2 +- ...qemuxml2argv-pseries-serial+console-native.args | 1 + .../qemuxml2argv-pseries-serial+console-native.xml | 18 ++ .../qemuxml2argv-pseries-serial-compat.args| 1 + .../qemuxml2argv-pseries-serial-compat.xml | 19 ++ ...qemuxml2argv-pseries-serial-invalid-machine.xml | 19 ++ ...rgs => qemuxml2ar
[libvirt] [PATCH v2 05/21] conf: Introduce virDomainChrTargetDefFormat()
Move formatting of the element for char devices out of
virDomainChrDefFormat() and into its own function.
Signed-off-by: Andrea Bolognani
---
src/conf/domain_conf.c | 67 ++
1 file changed, 40 insertions(+), 27 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e4c537136..224b88d9a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -23933,38 +23933,15 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
return -1;
}
+
static int
-virDomainChrDefFormat(virBufferPtr buf,
- virDomainChrDefPtr def,
- unsigned int flags)
+virDomainChrTargetDefFormat(virBufferPtr buf,
+const virDomainChrDef *def,
+unsigned int flags)
{
-const char *elementName = virDomainChrDeviceTypeToString(def->deviceType);
const char *targetType = virDomainChrTargetTypeToString(def->deviceType,
def->targetType);
-bool tty_compat;
-
-int ret = 0;
-
-if (!elementName) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected char device type %d"),
- def->deviceType);
-return -1;
-}
-virBufferAsprintf(buf, "<%s", elementName);
-virBufferAdjustIndent(buf, 2);
-tty_compat = (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
- def->target.port == 0 &&
- def->source->type == VIR_DOMAIN_CHR_TYPE_PTY &&
- !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
- def->source->data.file.path);
-if (virDomainChrAttrsDefFormat(buf, def->source, tty_compat) < 0)
-return -1;
-virBufferAddLit(buf, ">\n");
-virDomainChrSourceDefFormat(buf, def->source, flags);
-
-/* Format block */
switch (def->deviceType) {
case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: {
if (!targetType) {
@@ -24036,6 +24013,42 @@ virDomainChrDefFormat(virBufferPtr buf,
break;
}
+return 0;
+}
+
+
+static int
+virDomainChrDefFormat(virBufferPtr buf,
+ virDomainChrDefPtr def,
+ unsigned int flags)
+{
+const char *elementName = virDomainChrDeviceTypeToString(def->deviceType);
+bool tty_compat;
+
+int ret = 0;
+
+if (!elementName) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected char device type %d"),
+ def->deviceType);
+return -1;
+}
+
+virBufferAsprintf(buf, "<%s", elementName);
+virBufferAdjustIndent(buf, 2);
+tty_compat = (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
+ def->target.port == 0 &&
+ def->source->type == VIR_DOMAIN_CHR_TYPE_PTY &&
+ !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
+ def->source->data.file.path);
+if (virDomainChrAttrsDefFormat(buf, def->source, tty_compat) < 0)
+return -1;
+virBufferAddLit(buf, ">\n");
+virDomainChrSourceDefFormat(buf, def->source, flags);
+
+if (virDomainChrTargetDefFormat(buf, def, flags) < 0)
+ return -1;
+
virDomainDeviceInfoFormat(buf, &def->info, flags);
virBufferAdjustIndent(buf, -2);
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 02/21] conf: Run devicePostParse() again for the first serial device
The devicePostParse() callback is invoked for all devices so that
drivers have a chance to set their own specific values; however,
virDomainDefAddImplicitDevices() runs *after* the devicePostParse()
callbacks have been invoked and can add new devices, in which case
the driver wouldn't have a chance to customize them.
Work around the issue by invoking the devicePostParse() callback
after virDomainDefAddImplicitDevices(), only for the first serial
devices, which might have been added by it. The same was already
happening for the first video device for the very same reason.
This will become important later on, when we will change
virDomainDefAddConsoleCompat() not to set a targetType for
automatically added serial devices.
Signed-off-by: Andrea Bolognani
Reviewed-by: Pavel Hrdina
---
src/conf/domain_conf.c | 12
1 file changed, 12 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 969a6632b..20a4acd74 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4939,6 +4939,18 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
return -1;
}
+if (def->nserials != 0) {
+virDomainDeviceDef device = {
+.type = VIR_DOMAIN_DEVICE_CHR,
+.data.chr = def->serials[0],
+};
+
+/* serials[0] might have been added in AddImplicitDevices, after we've
+ * done the per-device post-parse */
+if (virDomainDefPostParseDeviceIterator(def, &device, NULL, data) < 0)
+return -1;
+}
+
/* clean up possibly duplicated metadata entries */
virXMLNodeSanitizeNamespaces(def->metadata);
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] qemu: implement element for devices
On Tue, Nov 21, 2017 at 10:52:48AM -0500, John Ferlan wrote: > > > On 11/21/2017 08:42 AM, Pavel Hrdina wrote: > > On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote: > >> > >> > >> On 11/14/2017 08:45 AM, Pavel Hrdina wrote: > >>> So far we were configuring the sound output based on what graphic device > >>> was configured in domain XML. This had a several disadvantages, it's > >>> not transparent, in case of multiple graphic devices it was overwritten > >>> by the last one and there was no simple way how to configure this per > >>> domain. > >>> > >>> The new element for devices allows you to configure > >>> which output will be used for each domain, however QEMU has a limitation > >>> that all sound devices will always use the same output because it is > >>> configured by environment variable QEMU_AUDIO_DRV per domain. > >>> > >>> For backward compatibility we need to preserve the defaults if no output > >>> is specified: > >>> > >>> - for vnc graphic it's by default NONE unless "vnc_allow_host_audio" > >>> was enabled, in that case we use DEFAULT which means it will pass > >>> the environment variable visible by libvirtd > >>> > >>> - for sdl graphic by default we pass the environment variable > >>> > >>> - for spice graphic we configure the SPICE output > >>> > >>> - if no graphic is configured we use by default NONE unless > >>> "nogfx_allow_host_audio" was enabled, in that case we pass > >>> the environment variable > >>> > >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1375433 > >>> > >>> Signed-off-by: Pavel Hrdina > >>> --- > >>> docs/formatdomain.html.in | 4 ++- > >>> src/qemu/qemu_command.c | 84 > >>> +-- > >>> src/qemu/qemu_domain.c| 54 ++ > >>> src/qemu/qemu_process.c | 41 +++ > >>> 4 files changed, 135 insertions(+), 48 deletions(-) > >>> > >> > >> Is there any way to make less things happen in one patch? Maybe even > >> the PostParse, Prepare, and Validate together? > > > > It would be probably possible to split this patch into two separate > > changes: > > > > 1. move the code out of command line generation into > > qemuProcessPrepareSound() > > > > 2. the rest of this patch > > > >> I need to look at this one with fresh eyes in the morning - it's > >> confusing at best especially since I don't find the functions self > >> documenting. > >> > >> I'm having trouble with the settings between PostParse and Prepare as > >> well as setting a default output which essentially changes an optional > >> parameter into a required one, doesn't it? I'm sure there's a harder and > >> even more confusing way to do things ;-). > > > > The PostParse function tries to find the first sound device with output > > configured (the first for loop) and sets this output to all other sound > > devices without any output configured (the second for loop). This is > > executed while parsing domain XML and implements the new feature. > > Understood, but it also changes each configured sound device that didn't > have defined to have the value from the one that did > have it. > > That would then be saved - something that would have been shown in the > qemuxml2xmltest output, e.g the multi output from patch 6 would be: > > > >function='0x0'/> > > > >function='0x0'/> > > > > I also note that comes after ... not that it matters, > but my brain recollects that is generally last for most > elements, although not required to be last - it just is generally. Yeah, semantically its fine, but it'd be better to stuck with our convention that address & alias are last. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/6] tests: add test cases for specific sound output
On 11/14/2017 08:45 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina > --- > ...xml2argv-sound-multi-different-output-spice.xml | 29 > ++ > .../qemuxml2argv-sound-multi-pa-output-spice.args | 26 +++ > .../qemuxml2argv-sound-multi-pa-output-spice.xml | 27 > .../qemuxml2argv-sound-pa-output-spice.args| 24 ++ > .../qemuxml2argv-sound-pa-output-spice.xml | 26 +++ > tests/qemuxml2argvtest.c | 12 + > 6 files changed, 144 insertions(+) > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-sound-multi-different-output-spice.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-sound-multi-pa-output-spice.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-sound-multi-pa-output-spice.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-sound-pa-output-spice.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-sound-pa-output-spice.xml > As pointed out elsewhere along the way - there's no qemuxml2xmltest being done. That's something we generally require when the domain conf and rng is adjusted. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] qemu: implement element for devices
On 11/21/2017 08:42 AM, Pavel Hrdina wrote: > On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote: >> >> >> On 11/14/2017 08:45 AM, Pavel Hrdina wrote: >>> So far we were configuring the sound output based on what graphic device >>> was configured in domain XML. This had a several disadvantages, it's >>> not transparent, in case of multiple graphic devices it was overwritten >>> by the last one and there was no simple way how to configure this per >>> domain. >>> >>> The new element for devices allows you to configure >>> which output will be used for each domain, however QEMU has a limitation >>> that all sound devices will always use the same output because it is >>> configured by environment variable QEMU_AUDIO_DRV per domain. >>> >>> For backward compatibility we need to preserve the defaults if no output >>> is specified: >>> >>> - for vnc graphic it's by default NONE unless "vnc_allow_host_audio" >>> was enabled, in that case we use DEFAULT which means it will pass >>> the environment variable visible by libvirtd >>> >>> - for sdl graphic by default we pass the environment variable >>> >>> - for spice graphic we configure the SPICE output >>> >>> - if no graphic is configured we use by default NONE unless >>> "nogfx_allow_host_audio" was enabled, in that case we pass >>> the environment variable >>> >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1375433 >>> >>> Signed-off-by: Pavel Hrdina >>> --- >>> docs/formatdomain.html.in | 4 ++- >>> src/qemu/qemu_command.c | 84 >>> +-- >>> src/qemu/qemu_domain.c| 54 ++ >>> src/qemu/qemu_process.c | 41 +++ >>> 4 files changed, 135 insertions(+), 48 deletions(-) >>> >> >> Is there any way to make less things happen in one patch? Maybe even >> the PostParse, Prepare, and Validate together? > > It would be probably possible to split this patch into two separate > changes: > > 1. move the code out of command line generation into > qemuProcessPrepareSound() > > 2. the rest of this patch > >> I need to look at this one with fresh eyes in the morning - it's >> confusing at best especially since I don't find the functions self >> documenting. >> >> I'm having trouble with the settings between PostParse and Prepare as >> well as setting a default output which essentially changes an optional >> parameter into a required one, doesn't it? I'm sure there's a harder and >> even more confusing way to do things ;-). > > The PostParse function tries to find the first sound device with output > configured (the first for loop) and sets this output to all other sound > devices without any output configured (the second for loop). This is > executed while parsing domain XML and implements the new feature. Understood, but it also changes each configured sound device that didn't have defined to have the value from the one that did have it. That would then be saved - something that would have been shown in the qemuxml2xmltest output, e.g the multi output from patch 6 would be: I also note that comes after ... not that it matters, but my brain recollects that is generally last for most elements, although not required to be last - it just is generally. In any case, I'm not sure it's "right" to change/add that output. It should be simple enough to ignore those that don't define some output. That was my point about making an optional element required. Being able to provide/determine some default when no sound device has an output defined would thus become the "job" of the Prepare API I think. > > The Prepare function is executed only while starting domain and > basically supplements the code removed from building command line. > It prepares only live definition that is about to be started so > the qemu command line code can only take the definition and convert > it into command line without any logic or without modifying the > live definition. > Right - these are the live entries not the config/saved defs - so to that degree altering things does make some sense. However, your choice to modify each live entry, but really only use the [0]th one in the command line building makes me want to consider whether it's better to create some sort of qemuDomainObjPrivate field instead. Since there can only be "one" (at this time) it would seem to be mechanism used elsewhere to keep track of QEMU private and specific shtuff. >> >> John >> >>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >>> index 3b7c367fc7..ae0d8b86be 100644 >>> --- a/docs/formatdomain.html.in >>> +++ b/docs/formatdomain.html.in >>> @@ -7064,7 +7064,9 @@ qemu-kvm -net nic,model=? /dev/null >>>the audio output is connected within host. There is mandatory >>>type attribute where valid values are 'none' to >>>disable the audio output, 'spice', 'pa', 'sdl', 'al
Re: [libvirt] [PATCH 02/12] qemu: command: Format frontend props with -device rather than -drive
On Mon, Nov 20, 2017 at 18:25:19 +0100, Peter Krempa wrote: > Historically we've formatted a lot of the attributes of a disk (disk > geometry, etc) with -drive. Since we use -device now, they should be > formatted there. > --- > src/qemu/qemu_command.c| 5 - > tests/qemuxml2argvdata/qemuxml2argv-disk-geometry.args | 6 +++--- > 2 files changed, 7 insertions(+), 4 deletions(-) So, SNACK to this. Qemu moved the parameters at some time but this does not do any capability checking so it may have broken older qemus. I'll strip this patch and the fallout from this series and just send the refactors. Later on I'll conditionally enable this when we start using -blockdev and thus these would be lost. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] qemu: Properly label and create evdev on input device hotplug
Utilize all the newly introduced function to create the evdev node
and label it on hotplug and destroy it on hotunplug.
This was forgotten in commits bc9ffaf and 67486bb.
https://bugzilla.redhat.com/show_bug.cgi?id=1509866
---
src/qemu/qemu_hotplug.c | 40 +---
1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 72a57d89e..fe69d42e8 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2746,7 +2746,11 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_INPUT,
{ .input = input } };
+virErrorPtr originalError = NULL;
bool releaseaddr = false;
+bool teardowndevice = false;
+bool teardownlabel = false;
+bool teardowncgroup = false;
if (input->bus != VIR_DOMAIN_INPUT_BUS_USB &&
input->bus != VIR_DOMAIN_INPUT_BUS_VIRTIO) {
@@ -2773,6 +2777,18 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
if (qemuBuildInputDevStr(&devstr, vm->def, input, priv->qemuCaps) < 0)
goto cleanup;
+if (qemuDomainNamespaceSetupInput(vm, input) < 0)
+goto cleanup;
+teardowndevice = true;
+
+if (qemuSetupInputCgroup(vm, input) < 0)
+goto cleanup;
+teardowncgroup = true;
+
+if (qemuSecuritySetInputLabel(vm, input) < 0)
+goto cleanup;
+teardownlabel = true;
+
if (VIR_REALLOC_N(vm->def->inputs, vm->def->ninputs + 1) < 0)
goto cleanup;
@@ -2788,14 +2804,23 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
VIR_APPEND_ELEMENT_COPY_INPLACE(vm->def->inputs, vm->def->ninputs, input);
ret = 0;
-releaseaddr = false;
audit:
virDomainAuditInput(vm, input, "attach", ret == 0);
cleanup:
-if (releaseaddr)
-qemuDomainReleaseDeviceAddress(vm, &input->info, NULL);
+if (ret < 0) {
+virErrorPreserveLast(&originalError);
+if (teardownlabel)
+qemuSecurityRestoreInputLabel(vm, input);
+if (teardowncgroup)
+qemuTeardownInputCgroup(vm, input);
+if (teardowndevice)
+qemuDomainNamespaceTeardownInput(vm, input);
+if (releaseaddr)
+qemuDomainReleaseDeviceAddress(vm, &input->info, NULL);
+virErrorRestore(&originalError);
+}
VIR_FREE(devstr);
return ret;
@@ -4283,6 +4308,15 @@ qemuDomainRemoveInputDevice(virDomainObjPtr vm,
break;
}
qemuDomainReleaseDeviceAddress(vm, &dev->info, NULL);
+if (qemuSecurityRestoreInputLabel(vm, dev) < 0)
+VIR_WARN("Unable to restore security label on input device");
+
+if (qemuTeardownInputCgroup(vm, dev) < 0)
+VIR_WARN("Unable to remove input device cgroup ACL");
+
+if (qemuDomainNamespaceTeardownInput(vm, dev) < 0)
+VIR_WARN("Unable to remove input device from /dev");
+
virDomainInputDefFree(vm->def->inputs[i]);
VIR_DELETE_ELEMENT(vm->def->inputs, i, vm->def->ninputs);
return 0;
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] qemu: functions for dealing with input device namespaces and labels
Introudce functions that will let us create the evdevs in namespaces
and label the devices on input device hotplug/hotunplug.
---
src/qemu/qemu_domain.c | 72
src/qemu/qemu_domain.h | 6
src/qemu/qemu_security.c | 58 ++
src/qemu/qemu_security.h | 6
4 files changed, 142 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b2fc3b816..5831a2025 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9969,6 +9969,78 @@ qemuDomainNamespaceTeardownRNG(virQEMUDriverPtr driver,
}
+int
+qemuDomainNamespaceSetupInput(virDomainObjPtr vm,
+ virDomainInputDefPtr input)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverPtr driver = priv->driver;
+virQEMUDriverConfigPtr cfg = NULL;
+char **devMountsPath = NULL;
+size_t ndevMountsPath = 0;
+const char *path = NULL;
+int ret = -1;
+
+if (!(path = virDomainInputDefGetPath(input)))
+return 0;
+
+if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
+return 0;
+
+cfg = virQEMUDriverGetConfig(driver);
+if (qemuDomainGetPreservedMounts(cfg, vm,
+ &devMountsPath, NULL,
+ &ndevMountsPath) < 0)
+goto cleanup;
+
+if (qemuDomainAttachDeviceMknod(driver, vm, path,
+devMountsPath, ndevMountsPath) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+virStringListFreeCount(devMountsPath, ndevMountsPath);
+virObjectUnref(cfg);
+return ret;
+}
+
+
+int
+qemuDomainNamespaceTeardownInput(virDomainObjPtr vm,
+ virDomainInputDefPtr input)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverPtr driver = priv->driver;
+virQEMUDriverConfigPtr cfg = NULL;
+char **devMountsPath = NULL;
+size_t ndevMountsPath = 0;
+const char *path = NULL;
+int ret = -1;
+
+if (!(path = virDomainInputDefGetPath(input)))
+return 0;
+
+if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
+return 0;
+
+cfg = virQEMUDriverGetConfig(driver);
+if (qemuDomainGetPreservedMounts(cfg, vm,
+ &devMountsPath, NULL,
+ &ndevMountsPath) < 0)
+goto cleanup;
+
+if (qemuDomainDetachDeviceUnlink(driver, vm, path,
+ devMountsPath, ndevMountsPath) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+virStringListFreeCount(devMountsPath, ndevMountsPath);
+virObjectUnref(cfg);
+return ret;
+}
+
+
/**
* qemuDomainDiskLookupByNodename:
* @def: domain definition to look for the disk
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index e021da51f..e699ab5ba 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -968,6 +968,12 @@ int qemuDomainNamespaceTeardownRNG(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainRNGDefPtr rng);
+int qemuDomainNamespaceSetupInput(virDomainObjPtr vm,
+ virDomainInputDefPtr input);
+
+int qemuDomainNamespaceTeardownInput(virDomainObjPtr vm,
+ virDomainInputDefPtr input);
+
virDomainDiskDefPtr qemuDomainDiskLookupByNodename(virDomainDefPtr def,
const char *nodename,
virStorageSourcePtr *src,
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 6fc3b0bb6..e7d2bbd5a 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -306,3 +306,61 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver,
virSecurityManagerTransactionAbort(driver->securityManager);
return ret;
}
+
+
+int
+qemuSecuritySetInputLabel(virDomainObjPtr vm,
+ virDomainInputDefPtr input)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverPtr driver = priv->driver;
+int ret = -1;
+
+if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+virSecurityManagerTransactionStart(driver->securityManager) < 0)
+goto cleanup;
+
+if (virSecurityManagerSetInputLabel(driver->securityManager,
+vm->def,
+input) < 0)
+goto cleanup;
+
+if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+virSecurityManagerTransactionCommit(driver->securityManager,
+vm->pid) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+virSecurityManagerTransactionAbort(driver->securityManager);
+return ret;
+}
+
+
+int
+qemuSecurityRestoreInputLabel(virDomainObjPtr vm,
+ vir
[libvirt] [PATCH 3/5] qemu: Introduce functions for input device cgroup manipulation
Export qemuSetupInputCgroup and introduce qemuTeardownInputCgroup
for hotunplug.
---
src/qemu/qemu_cgroup.c | 25 -
src/qemu/qemu_cgroup.h | 4
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 0f75e22f9..19252ea23 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -246,7 +246,7 @@ qemuSetupTPMCgroup(virDomainObjPtr vm)
}
-static int
+int
qemuSetupInputCgroup(virDomainObjPtr vm,
virDomainInputDefPtr dev)
{
@@ -270,6 +270,29 @@ qemuSetupInputCgroup(virDomainObjPtr vm,
int
+qemuTeardownInputCgroup(virDomainObjPtr vm,
+virDomainInputDefPtr dev)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+int ret = 0;
+
+if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
+return 0;
+
+switch (dev->type) {
+case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
+VIR_DEBUG("Process path '%s' for input device", dev->source.evdev);
+ret = virCgroupDenyDevicePath(priv->cgroup, dev->source.evdev,
+ VIR_CGROUP_DEVICE_RWM, false);
+virDomainAuditCgroupPath(vm, priv->cgroup, "deny", dev->source.evdev,
"rwm", ret == 0);
+break;
+}
+
+return ret;
+}
+
+
+int
qemuSetupHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
index 3fc158361..3b8ff6055 100644
--- a/src/qemu/qemu_cgroup.h
+++ b/src/qemu/qemu_cgroup.h
@@ -37,6 +37,10 @@ int qemuSetupDiskCgroup(virDomainObjPtr vm,
virDomainDiskDefPtr disk);
int qemuTeardownDiskCgroup(virDomainObjPtr vm,
virDomainDiskDefPtr disk);
+int qemuSetupInputCgroup(virDomainObjPtr vm,
+ virDomainInputDefPtr dev);
+int qemuTeardownInputCgroup(virDomainObjPtr vm,
+virDomainInputDefPtr dev);
int qemuSetupHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
ATTRIBUTE_RETURN_CHECK;
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] Introduce virDomainInputDefGetPath
Use it to denadify qemuDomainSetupInput.
---
src/conf/domain_conf.c | 16
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 21 -
4 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 969a6632b..5d0290d07 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1404,6 +1404,22 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr
def)
VIR_FREE(def);
}
+const char *virDomainInputDefGetPath(virDomainInputDefPtr input)
+{
+switch ((virDomainInputType) input->type) {
+case VIR_DOMAIN_INPUT_TYPE_MOUSE:
+case VIR_DOMAIN_INPUT_TYPE_TABLET:
+case VIR_DOMAIN_INPUT_TYPE_KBD:
+case VIR_DOMAIN_INPUT_TYPE_LAST:
+return NULL;
+break;
+
+case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
+return input->source.evdev;
+}
+return NULL;
+}
+
void virDomainInputDefFree(virDomainInputDefPtr def)
{
if (!def)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index cb8701dd2..e8ab9abdf 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2706,6 +2706,7 @@ int virDomainObjWaitUntil(virDomainObjPtr vm,
void virDomainPanicDefFree(virDomainPanicDefPtr panic);
void virDomainResourceDefFree(virDomainResourceDefPtr resource);
void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def);
+const char *virDomainInputDefGetPath(virDomainInputDefPtr input);
void virDomainInputDefFree(virDomainInputDefPtr def);
virDomainDiskDefPtr virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt);
void virDomainDiskDefFree(virDomainDiskDefPtr def);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d3ca6b2ec..2997a469d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -395,6 +395,7 @@ virDomainHypervTypeToString;
virDomainInputBusTypeToString;
virDomainInputDefFind;
virDomainInputDefFree;
+virDomainInputDefGetPath;
virDomainIOMMUModelTypeFromString;
virDomainIOMMUModelTypeToString;
virDomainIOThreadIDAdd;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cc7596bad..b2fc3b816 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8949,25 +8949,12 @@ qemuDomainSetupInput(virQEMUDriverConfigPtr cfg
ATTRIBUTE_UNUSED,
virDomainInputDefPtr input,
const struct qemuDomainCreateDeviceData *data)
{
-int ret = -1;
-
-switch ((virDomainInputType) input->type) {
-case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
-if (qemuDomainCreateDevice(input->source.evdev, data, false) < 0)
-goto cleanup;
-break;
+const char *path = virDomainInputDefGetPath(input);
-case VIR_DOMAIN_INPUT_TYPE_MOUSE:
-case VIR_DOMAIN_INPUT_TYPE_TABLET:
-case VIR_DOMAIN_INPUT_TYPE_KBD:
-case VIR_DOMAIN_INPUT_TYPE_LAST:
-/* nada */
-break;
-}
+if (path && qemuDomainCreateDevice(path, data, false) < 0)
+return -1;
-ret = 0;
- cleanup:
-return ret;
+return 0;
}
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/5] Fix labeling for evdev passthrough hotplug
https://bugzilla.redhat.com/show_bug.cgi?id=1509866 Ján Tomko (5): Introduce virDomainInputDefGetPath security: Introduce functions for input device hot(un)plug qemu: Introduce functions for input device cgroup manipulation qemu: functions for dealing with input device namespaces and labels qemu: Properly label and create evdev on input device hotplug src/conf/domain_conf.c | 16 +++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms| 3 ++ src/qemu/qemu_cgroup.c | 25 ++- src/qemu/qemu_cgroup.h | 4 ++ src/qemu/qemu_domain.c | 93 + src/qemu/qemu_domain.h | 6 +++ src/qemu/qemu_hotplug.c | 40 -- src/qemu/qemu_security.c| 58 + src/qemu/qemu_security.h| 6 +++ src/security/security_dac.c | 3 ++ src/security/security_driver.h | 9 src/security/security_manager.c | 36 src/security/security_manager.h | 8 src/security/security_nop.c | 11 + src/security/security_selinux.c | 3 ++ src/security/security_stack.c | 38 + 17 files changed, 339 insertions(+), 21 deletions(-) -- 2.13.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] security: Introduce functions for input device hot(un)plug
Export the existing DAC and SELinux for separate use and introduce
functions for stack, nop and the security manager.
---
src/libvirt_private.syms| 2 ++
src/security/security_dac.c | 3 +++
src/security/security_driver.h | 9 +
src/security/security_manager.c | 36
src/security/security_manager.h | 8
src/security/security_nop.c | 11 +++
src/security/security_selinux.c | 3 +++
src/security/security_stack.c | 38 ++
8 files changed, 110 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2997a469d..31969a092 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1274,6 +1274,7 @@ virSecurityManagerRestoreAllLabel;
virSecurityManagerRestoreDiskLabel;
virSecurityManagerRestoreHostdevLabel;
virSecurityManagerRestoreImageLabel;
+virSecurityManagerRestoreInputLabel;
virSecurityManagerRestoreMemoryLabel;
virSecurityManagerRestoreSavedStateLabel;
virSecurityManagerSetAllLabel;
@@ -1283,6 +1284,7 @@ virSecurityManagerSetDiskLabel;
virSecurityManagerSetHostdevLabel;
virSecurityManagerSetImageFDLabel;
virSecurityManagerSetImageLabel;
+virSecurityManagerSetInputLabel;
virSecurityManagerSetMemoryLabel;
virSecurityManagerSetProcessLabel;
virSecurityManagerSetSavedStateLabel;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 54120890f..52ca07a10 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -2123,6 +2123,9 @@ virSecurityDriver virSecurityDriverDAC = {
.domainSetSecurityMemoryLabel = virSecurityDACSetMemoryLabel,
.domainRestoreSecurityMemoryLabel = virSecurityDACRestoreMemoryLabel,
+.domainSetSecurityInputLabel= virSecurityDACSetInputLabel,
+.domainRestoreSecurityInputLabel= virSecurityDACRestoreInputLabel,
+
.domainSetSecurityDaemonSocketLabel = virSecurityDACSetDaemonSocketLabel,
.domainSetSecuritySocketLabel = virSecurityDACSetSocketLabel,
.domainClearSecuritySocketLabel = virSecurityDACClearSocketLabel,
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 0b3b45248..1b3070d06 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -131,6 +131,12 @@ typedef int (*virSecurityDomainSetMemoryLabel)
(virSecurityManagerPtr mgr,
typedef int (*virSecurityDomainRestoreMemoryLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
virDomainMemoryDefPtr mem);
+typedef int (*virSecurityDomainSetInputLabel) (virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virDomainInputDefPtr input);
+typedef int (*virSecurityDomainRestoreInputLabel) (virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virDomainInputDefPtr input);
typedef int (*virSecurityDomainSetPathLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
const char *path);
@@ -163,6 +169,9 @@ struct _virSecurityDriver {
virSecurityDomainSetMemoryLabel domainSetSecurityMemoryLabel;
virSecurityDomainRestoreMemoryLabel domainRestoreSecurityMemoryLabel;
+virSecurityDomainSetInputLabel domainSetSecurityInputLabel;
+virSecurityDomainRestoreInputLabel domainRestoreSecurityInputLabel;
+
virSecurityDomainSetDaemonSocketLabel domainSetSecurityDaemonSocketLabel;
virSecurityDomainSetSocketLabel domainSetSecuritySocketLabel;
virSecurityDomainClearSocketLabel domainClearSecuritySocketLabel;
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 60cfc92e7..3cf12188a 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1116,3 +1116,39 @@
virSecurityManagerRestoreMemoryLabel(virSecurityManagerPtr mgr,
virReportUnsupportedError();
return -1;
}
+
+
+int
+virSecurityManagerSetInputLabel(virSecurityManagerPtr mgr,
+virDomainDefPtr vm,
+virDomainInputDefPtr input)
+{
+if (mgr->drv->domainSetSecurityInputLabel) {
+int ret;
+virObjectLock(mgr);
+ret = mgr->drv->domainSetSecurityInputLabel(mgr, vm, input);
+virObjectUnlock(mgr);
+return ret;
+}
+
+virReportUnsupportedError();
+return -1;
+}
+
+
+int
+virSecurityManagerRestoreInputLabel(virSecurityManagerPtr mgr,
+virDomainDefPtr vm,
+virDomainInputDefPtr input)
+{
+if (mgr->drv->domainRestoreSecurityInputLabel) {
+int ret;
+virObjectLock(mgr);
+ret = mgr->drv->domainRestore
Re: [libvirt] [PATCH 4/6] conf: introduce element for devices
On 11/21/2017 08:12 AM, Pavel Hrdina wrote: > On Mon, Nov 20, 2017 at 06:04:27PM -0500, John Ferlan wrote: >> >> >> On 11/14/2017 08:45 AM, Pavel Hrdina wrote: >>> So far it was not possible to specify how the audio output from guest >> >> s/So far it was/It is/ >> >>> should be presented to host/users. Now it will be possible to do so >>> via element for device where you specify the output >>> "type". >>> >>> Signed-off-by: Pavel Hrdina >>> --- >>> docs/formatdomain.html.in | 9 +++ >>> docs/schemas/domaincommon.rng | 14 ++ >>> src/conf/domain_conf.c| 61 >>> +++ >>> src/conf/domain_conf.h| 14 ++ >>> src/libvirt_private.syms | 2 ++ >>> 5 files changed, 100 insertions(+) >>> >>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >>> index 47c43d0666..3b7c367fc7 100644 >>> --- a/docs/formatdomain.html.in >>> +++ b/docs/formatdomain.html.in >>> @@ -7058,6 +7058,15 @@ qemu-kvm -net nic,model=? /dev/null >>>slot, documented above. >>> >>> >>> + >>> + Since 3.10.0 sound device can have >> >> s/sound/, a sound/ >> >>> + an optional output element which configures where >>> + the audio output is connected within host. There is mandatory >>> + type attribute where valid values are 'none' to >>> + disable the audio output, 'spice', 'pa', 'sdl', 'alsa', 'oss'. >> >> I've become preferential to seeing these in a list. I have no idea based >> on the text here what 'spice', ... 'oss' really means. IOW: Why would I >> want to set to something else - what does it do/provide? > > I'm not sure how to describe it more than providing links to all the > projects. SPICE is the only unique output since the audio stream is > sent over SPICE protocol, but the remaining outputs are IMHO > self-explanatory because the are standard linux/unix audio > layers/libraries. I was reading literally vis-a-vis a list of supported values and there was something missing. I was not thinking in terms that the list is/was from the standard linux/unix audio layers/libraries. As I've learned since originally looking at this - the overall design of graphics/sound is a bit, well, odd/unusual... Anwyay restated: There is mandatory type attribute where valid values to enable the audio output are 'spice', 'pa', 'sdl', 'alsa', or 'oss'. Use 'none' in order to disable the audio output. With a bit more thought on this now - why would someone add a sound card, but select an output of 'none'? Why even add the sound card then? You have a sound card that doesn't emit sound anywhere? Why have it?! And in summary, it seems to me now that what the 'output' is designed to do is supply the driver for the QEMU_AUDIO_DRV environment variable, true? To override what may be globally set? > > Describing all the differences is out of scope of our documentation. > Hmmm.. maybe I wasn't clear enough - it wasn't the differences that I cared about it was the what are those shorthand acronyms. Someone reading the list literally may not know what they are. I certainly don't think open sound system (I had to look it up)... So pa is perhaps a known synonym for pulseaudio... >> >>> + This might not be supported by all hypervisors. >> >> True, but perhaps also true of many other things, too. > > I can change it to "Currently supported only by QEMU driver." > OK, so this makes sense in light of what appears to be the goal - to provide something for the env variable. BTW: Probably should add the xml2xml parsing tests to this patch - although they are present later - it is something important to ask of any XML changes. John >>> + >>> + >>> Watchdog device >>> >>> >> >> >> Reviewed-by: John Ferlan >> >> John >> >> [...] >> >> -- >> libvir-list mailing list >> libvir-list@redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] conf: introduce element for devices
On Tue, Nov 21, 2017 at 01:25:33PM +, Daniel P. Berrange wrote:
> On Tue, Nov 14, 2017 at 02:45:09PM +0100, Pavel Hrdina wrote:
> > So far it was not possible to specify how the audio output from guest
> > should be presented to host/users. Now it will be possible to do so
> > via element for device where you specify the output
> > "type".
> >
> > Signed-off-by: Pavel Hrdina
> > ---
> > docs/formatdomain.html.in | 9 +++
> > docs/schemas/domaincommon.rng | 14 ++
> > src/conf/domain_conf.c| 61
> > +++
> > src/conf/domain_conf.h| 14 ++
> > src/libvirt_private.syms | 2 ++
> > 5 files changed, 100 insertions(+)
> >
>
> > +static int
> > +virDomainSoundOutputParseXML(xmlXPathContextPtr ctxt,
> > + virDomainSoundDefPtr sound)
> > +{
> > +int ret = -1;
> > +char *type = NULL;
> > +int typeVal;
> > +xmlNodePtr *outputNodes = NULL;
> > +int noutputs;
> > +
> > +noutputs = virXPathNodeSet("./output", ctxt, &outputNodes);
> > +if (noutputs < 0)
> > +return -1;
> > +
> > +if (noutputs > 1) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("sound device can have only one output
> > configured"));
> > +goto cleanup;
> > +}
>
> The implication of this is that if you have multiple display backends enabled
> only one of them will ever be selectable as the audio backend. It is not
> unreasonable to consider that audio should go to all backends, or depending
> on which is in use. eg if QEMU has SDL + SPICE enabled, audio could
> conceptually go to SDL by default, and get dynamically switched to SPICE
> whenver there is a SPICE client actively connected. This is a risk of
> trying to design a configuration approach for this, without us asking
> QEMU to consider their corresponding design possibilities
Having this limitation now doesn't mean that we cannot remove it in the
future if QEMU implements a way how to configure multiple audio
backends. I can move it into qemuValidateCallback to make it QEMU
specific and if we need to remove it we can do so.
Pavel
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] qemu: implement element for devices
On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote:
>
>
> On 11/14/2017 08:45 AM, Pavel Hrdina wrote:
> > So far we were configuring the sound output based on what graphic device
> > was configured in domain XML. This had a several disadvantages, it's
> > not transparent, in case of multiple graphic devices it was overwritten
> > by the last one and there was no simple way how to configure this per
> > domain.
> >
> > The new element for devices allows you to configure
> > which output will be used for each domain, however QEMU has a limitation
> > that all sound devices will always use the same output because it is
> > configured by environment variable QEMU_AUDIO_DRV per domain.
> >
> > For backward compatibility we need to preserve the defaults if no output
> > is specified:
> >
> > - for vnc graphic it's by default NONE unless "vnc_allow_host_audio"
> > was enabled, in that case we use DEFAULT which means it will pass
> > the environment variable visible by libvirtd
> >
> > - for sdl graphic by default we pass the environment variable
> >
> > - for spice graphic we configure the SPICE output
> >
> > - if no graphic is configured we use by default NONE unless
> > "nogfx_allow_host_audio" was enabled, in that case we pass
> > the environment variable
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1375433
> >
> > Signed-off-by: Pavel Hrdina
> > ---
> > docs/formatdomain.html.in | 4 ++-
> > src/qemu/qemu_command.c | 84
> > +--
> > src/qemu/qemu_domain.c| 54 ++
> > src/qemu/qemu_process.c | 41 +++
> > 4 files changed, 135 insertions(+), 48 deletions(-)
> >
>
> Is there any way to make less things happen in one patch? Maybe even
> the PostParse, Prepare, and Validate together?
It would be probably possible to split this patch into two separate
changes:
1. move the code out of command line generation into
qemuProcessPrepareSound()
2. the rest of this patch
> I need to look at this one with fresh eyes in the morning - it's
> confusing at best especially since I don't find the functions self
> documenting.
>
> I'm having trouble with the settings between PostParse and Prepare as
> well as setting a default output which essentially changes an optional
> parameter into a required one, doesn't it? I'm sure there's a harder and
> even more confusing way to do things ;-).
The PostParse function tries to find the first sound device with output
configured (the first for loop) and sets this output to all other sound
devices without any output configured (the second for loop). This is
executed while parsing domain XML and implements the new feature.
The Prepare function is executed only while starting domain and
basically supplements the code removed from building command line.
It prepares only live definition that is about to be started so
the qemu command line code can only take the definition and convert
it into command line without any logic or without modifying the
live definition.
>
> John
>
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 3b7c367fc7..ae0d8b86be 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -7064,7 +7064,9 @@ qemu-kvm -net nic,model=? /dev/null
> >the audio output is connected within host. There is mandatory
> >type attribute where valid values are 'none' to
> >disable the audio output, 'spice', 'pa', 'sdl', 'alsa', 'oss'.
> > - This might not be supported by all hypervisors.
> > + This might not be supported by all hypervisors. QEMU driver
> > + has a limitation that all sound devices have to use the same
> > + output.
> >
> >
> > Watchdog device
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index c5c7bd7e54..5cbd1d0d46 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4442,67 +4442,57 @@ qemuBuildSoundCodecStr(virDomainSoundDefPtr sound,
> > }
> >
> >
> > -static void
> > +static int
> > qemuBuildSoundAudioEnv(virCommandPtr cmd,
> > - const virDomainDef *def,
> > - virQEMUDriverConfigPtr cfg)
> > + const virDomainDef *def)
> > {
> > +char *envStr = NULL;
> > +
> > if (def->nsounds == 0) {
> > virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
> > -return;
> > +return 0;
> > }
> >
> > -if (def->ngraphics == 0) {
> > -if (cfg->nogfxAllowHostAudio)
> > -virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
> > -else
> > -virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
> > -} else {
> > -switch (def->graphics[def->ngraphics - 1]->type) {
> > -case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
> > -/* If using SDL for video, then we should just
Re: [libvirt] [PATCH 4/6] conf: introduce element for devices
On Tue, Nov 14, 2017 at 02:45:09PM +0100, Pavel Hrdina wrote:
> So far it was not possible to specify how the audio output from guest
> should be presented to host/users. Now it will be possible to do so
> via element for device where you specify the output
> "type".
>
> Signed-off-by: Pavel Hrdina
> ---
> docs/formatdomain.html.in | 9 +++
> docs/schemas/domaincommon.rng | 14 ++
> src/conf/domain_conf.c| 61
> +++
> src/conf/domain_conf.h| 14 ++
> src/libvirt_private.syms | 2 ++
> 5 files changed, 100 insertions(+)
>
> +static int
> +virDomainSoundOutputParseXML(xmlXPathContextPtr ctxt,
> + virDomainSoundDefPtr sound)
> +{
> +int ret = -1;
> +char *type = NULL;
> +int typeVal;
> +xmlNodePtr *outputNodes = NULL;
> +int noutputs;
> +
> +noutputs = virXPathNodeSet("./output", ctxt, &outputNodes);
> +if (noutputs < 0)
> +return -1;
> +
> +if (noutputs > 1) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("sound device can have only one output
> configured"));
> +goto cleanup;
> +}
The implication of this is that if you have multiple display backends enabled
only one of them will ever be selectable as the audio backend. It is not
unreasonable to consider that audio should go to all backends, or depending
on which is in use. eg if QEMU has SDL + SPICE enabled, audio could
conceptually go to SDL by default, and get dynamically switched to SPICE
whenver there is a SPICE client actively connected. This is a risk of
trying to design a configuration approach for this, without us asking
QEMU to consider their corresponding design possibilities
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] conf: introduce element for devices
On Mon, Nov 20, 2017 at 06:04:27PM -0500, John Ferlan wrote: > > > On 11/14/2017 08:45 AM, Pavel Hrdina wrote: > > So far it was not possible to specify how the audio output from guest > > s/So far it was/It is/ > > > should be presented to host/users. Now it will be possible to do so > > via element for device where you specify the output > > "type". > > > > Signed-off-by: Pavel Hrdina > > --- > > docs/formatdomain.html.in | 9 +++ > > docs/schemas/domaincommon.rng | 14 ++ > > src/conf/domain_conf.c| 61 > > +++ > > src/conf/domain_conf.h| 14 ++ > > src/libvirt_private.syms | 2 ++ > > 5 files changed, 100 insertions(+) > > > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > > index 47c43d0666..3b7c367fc7 100644 > > --- a/docs/formatdomain.html.in > > +++ b/docs/formatdomain.html.in > > @@ -7058,6 +7058,15 @@ qemu-kvm -net nic,model=? /dev/null > >slot, documented above. > > > > > > + > > + Since 3.10.0 sound device can have > > s/sound/, a sound/ > > > + an optional output element which configures where > > + the audio output is connected within host. There is mandatory > > + type attribute where valid values are 'none' to > > + disable the audio output, 'spice', 'pa', 'sdl', 'alsa', 'oss'. > > I've become preferential to seeing these in a list. I have no idea based > on the text here what 'spice', ... 'oss' really means. IOW: Why would I > want to set to something else - what does it do/provide? I'm not sure how to describe it more than providing links to all the projects. SPICE is the only unique output since the audio stream is sent over SPICE protocol, but the remaining outputs are IMHO self-explanatory because the are standard linux/unix audio layers/libraries. Describing all the differences is out of scope of our documentation. > > > + This might not be supported by all hypervisors. > > True, but perhaps also true of many other things, too. I can change it to "Currently supported only by QEMU driver." > > + > > + > > Watchdog device > > > > > > > Reviewed-by: John Ferlan > > John > > [...] > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] qemu: move QEMU_AUDIO_DRIVER out of graphic into sound
On 11/21/2017 03:59 AM, Pavel Hrdina wrote:
> On Mon, Nov 20, 2017 at 05:39:38PM -0500, John Ferlan wrote:
>>
>>
>> On 11/14/2017 08:45 AM, Pavel Hrdina wrote:
>>> Setting the default audio output depends on specific graphic device
>>> but requires having sound device configured as well and it's the sound
>>> device that handles the audio.
>>>
>>> Signed-off-by: Pavel Hrdina
>>> ---
>>> src/qemu/qemu_command.c| 84
>>> +++---
>>> .../qemuxml2argv-clock-france.args | 2 +-
>>> 2 files changed, 58 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index eb72db33ba..e1ef1b05fa 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -4442,10 +4442,62 @@ qemuBuildSoundCodecStr(virDomainSoundDefPtr sound,
>>> }
>>> >
>>> +static void
>>> +qemuBuildSoundAudioEnv(virCommandPtr cmd,
>>> + const virDomainDef *def,
>>> + virQEMUDriverConfigPtr cfg)
>>> +{
>>> +if (def->ngraphics == 0) {
>>> +if (cfg->nogfxAllowHostAudio)
>>> +virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
>>> +else
>>> +virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
>>> +} else {
>>> +switch (def->graphics[def->ngraphics - 1]->type) {
>>
>> So it's the "last" graphics device that then defines "how" this all
>> works? Makes sense for QEMU_AUDIO_DRV since whichever is last would be
>> the winner and get the chicken dinner, but...
>>
>>> +case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
>>> +/* If using SDL for video, then we should just let it
>>> + * use QEMU's host audio drivers, possibly SDL too
>>> + * User can set these two before starting libvirtd
>>> + */
>>> +virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
>>> +virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL);
>>
>> ... if there was more than one graphics device defined, where one was
>> SDL and it wasn't the last one, then theoretically at least this would
>> not be defined.
>
> This is intentional, I should have mentioned it in the commit message.
> The original design was just wrong, nothing else. Setting
> "SDL_AUDIODRIVER" if the SDL audio output is not used is pointless
> and we shouldn't do it. I can move this change to separate patch.
>
> Pavel
>
I figured you had gone through the history - I certain hadn't ;-)... I
would say by this point in the review I was still "missing" the final
detail regarding the bug you're working on O:) - that the audio was
being 'tied to' that last device only. Guess I was thinking it could
somehow be magically applied to "any" device.
Anyway, long way of saying - it's fine here. Adding a bit more detail to
the commit message will help though.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] tests: add test cases for default sound output
On 11/21/2017 03:38 AM, Pavel Hrdina wrote: > On Mon, Nov 20, 2017 at 05:22:48PM -0500, John Ferlan wrote: >> >> >> On 11/14/2017 08:45 AM, Pavel Hrdina wrote: >>> These test cases models current situation where there is no way how >>> to specify sound output and that it's based on which graphic device >>> is the last one. >>> >> >> Personally had a hard time parsing what the commit message said, but >> based on what happens 2 patches from now, I think you're just trying to >> add vm's w/ a sound device. Not so sure it's the "default" sound device >> or just "a" sound device. Not sure how much clearer you could make >> things or if it really matters - I'll leave it up to you to reread and >> be sure what's happening is what you expect! > > Well I'm sure that this patch does what I expect from it :). > > How about this commit message: > > These test cases models current situation where there is no way how s/models/model the/ s/situation/environment/ s/way how/mechanism/ > to specify audio output of sound devices. The audio output is s/audio/the audio/ > currently based on which graphic device is configured as the last > one in domain XML. > > Pavel > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: explicitly disable audio if there is no sound device
On Tue, Nov 21, 2017 at 01:21:26PM +0100, Pavel Hrdina wrote: > On Tue, Nov 21, 2017 at 12:09:54PM +, Daniel P. Berrange wrote: > > On Tue, Nov 21, 2017 at 01:02:05PM +0100, Peter Krempa wrote: > > > On Tue, Nov 21, 2017 at 11:48:20 +, Daniel Berrange wrote: > > > > On Tue, Nov 21, 2017 at 12:44:44PM +0100, Peter Krempa wrote: > > > > > On Tue, Nov 21, 2017 at 11:24:06 +, Daniel Berrange wrote: > > > > > > IOW, I don't think this patch is desirable. > > > > > > > > > > We could allow hotplug only if qemu will allow to specify the sound > > > > > output per-soundcard which would avoid this problem. > > > > > > > > I agree that having ability to configure distinct outputs per sound card > > > > might be useful, I don't think it is a blocking feature for hotplugging > > > > sound cards. ie, there's no reason why a user should not be able to > > > > unplug their current sound card, and plug in a new sound card for a > > > > running guests - they would only ever have 1 sound card present at > > > > a time in that scenario, so distinct outputs is not a requirement > > > > for that usecase. > > > > > > While not a requirement, mandating that new features are available only > > > with new software sometimes saves a lot of headaches. I would not > > > mind if sound device hotplug will be supported only when specific > > > outputs can be selected when adding the sound card and would not work > > > without that. > > > > > > Unfortunately qemu does not yet support this and I don't think that they > > > will any soon. > > > > Agreed, and the most interesting thing for multiple audio cards is > > probably multi-seat support, which wouldn't need choosing between > > different backends, instead choosing different spice/vnc instances. > > > > Basically, I think this patch should be dropped. > > Ok, I don't mind dropping this patch. At least it pointed out that > QEMU is design is broken. This patch only affects use case where > the domain is started without any sound device and I personally don't > care if the audio output will be basically random unless you have > spice graphic configured. > > However, if we enabled hot-plugging sound device we need to make sure > that we don't allow configuring sound output until we have a decent way > how to configure it. Agreed - that's pretty much why i never exposed sound backend in the XML and instead hardcoded it with optional override in qemu.conf when first doing this Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] vsh: Make self-test more robust
On 11/21/2017 10:25 AM, Erik Skultety wrote: > On Thu, Nov 16, 2017 at 02:49:27PM +0100, Michal Privoznik wrote: >> There are couple of limitations when it comes to option types and >> flags for the options. For instance, VSH_OT_STRING cannot have >> VSH_OFLAG_REQ set (commit c7543a728). For some reason this is >> checked in vshCmddefHelp() but not in vshCmddefCheckInternals(). >> >> Signed-off-by: Michal Privoznik >> --- > > [...] > >> -if (opt->type == VSH_OT_ARGV && cmd->opts[i + 1].name) >> -return -1; /* argv option must be listed last */ >> +break; >> +case VSH_OT_ARGV: >> +if (cmd->opts[i + 1].name) >> +return -1; /* argv option must be listed last */ >> +break; >> + >> +case VSH_OT_DATA: >> +if (!(opt->flags & VSH_OFLAG_REQ)) >> +return -1; /* OT_DATA should always be required. */ > > This got me thinking a bit, since we're going to do the checking here, is > there > a need for performing the same check within vshCmddefHelp too? My reasoning is > that virsh-self-test is part of the test suite run at make check. Not a deal > breaker though, just thinking out loud. Yeah, it probably doesn't. But frankly, I don't know why we have any check at vshCmdDefHelp in the first place. Maybe it used to be a test case? Like we ran all the commands with --help? Anyway, it doesn't make sense now so I'll remove it. > >> +break; >> + >> +case VSH_OT_INT: >> +/* nada */ > > please don't... ¿Por qué no? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: explicitly disable audio if there is no sound device
On Tue, Nov 21, 2017 at 12:09:54PM +, Daniel P. Berrange wrote: > On Tue, Nov 21, 2017 at 01:02:05PM +0100, Peter Krempa wrote: > > On Tue, Nov 21, 2017 at 11:48:20 +, Daniel Berrange wrote: > > > On Tue, Nov 21, 2017 at 12:44:44PM +0100, Peter Krempa wrote: > > > > On Tue, Nov 21, 2017 at 11:24:06 +, Daniel Berrange wrote: > > > > > IOW, I don't think this patch is desirable. > > > > > > > > We could allow hotplug only if qemu will allow to specify the sound > > > > output per-soundcard which would avoid this problem. > > > > > > I agree that having ability to configure distinct outputs per sound card > > > might be useful, I don't think it is a blocking feature for hotplugging > > > sound cards. ie, there's no reason why a user should not be able to > > > unplug their current sound card, and plug in a new sound card for a > > > running guests - they would only ever have 1 sound card present at > > > a time in that scenario, so distinct outputs is not a requirement > > > for that usecase. > > > > While not a requirement, mandating that new features are available only > > with new software sometimes saves a lot of headaches. I would not > > mind if sound device hotplug will be supported only when specific > > outputs can be selected when adding the sound card and would not work > > without that. > > > > Unfortunately qemu does not yet support this and I don't think that they > > will any soon. > > Agreed, and the most interesting thing for multiple audio cards is > probably multi-seat support, which wouldn't need choosing between > different backends, instead choosing different spice/vnc instances. > > Basically, I think this patch should be dropped. Ok, I don't mind dropping this patch. At least it pointed out that QEMU is design is broken. This patch only affects use case where the domain is started without any sound device and I personally don't care if the audio output will be basically random unless you have spice graphic configured. However, if we enabled hot-plugging sound device we need to make sure that we don't allow configuring sound output until we have a decent way how to configure it. Pavel signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: explicitly disable audio if there is no sound device
On Tue, Nov 21, 2017 at 01:02:05PM +0100, Peter Krempa wrote: > On Tue, Nov 21, 2017 at 11:48:20 +, Daniel Berrange wrote: > > On Tue, Nov 21, 2017 at 12:44:44PM +0100, Peter Krempa wrote: > > > On Tue, Nov 21, 2017 at 11:24:06 +, Daniel Berrange wrote: > > > > IOW, I don't think this patch is desirable. > > > > > > We could allow hotplug only if qemu will allow to specify the sound > > > output per-soundcard which would avoid this problem. > > > > I agree that having ability to configure distinct outputs per sound card > > might be useful, I don't think it is a blocking feature for hotplugging > > sound cards. ie, there's no reason why a user should not be able to > > unplug their current sound card, and plug in a new sound card for a > > running guests - they would only ever have 1 sound card present at > > a time in that scenario, so distinct outputs is not a requirement > > for that usecase. > > While not a requirement, mandating that new features are available only > with new software sometimes saves a lot of headaches. I would not > mind if sound device hotplug will be supported only when specific > outputs can be selected when adding the sound card and would not work > without that. > > Unfortunately qemu does not yet support this and I don't think that they > will any soon. Agreed, and the most interesting thing for multiple audio cards is probably multi-seat support, which wouldn't need choosing between different backends, instead choosing different spice/vnc instances. Basically, I think this patch should be dropped. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: explicitly disable audio if there is no sound device
On Tue, Nov 21, 2017 at 12:57:36PM +0100, Pavel Hrdina wrote: > On Tue, Nov 21, 2017 at 11:24:06AM +, Daniel P. Berrange wrote: > > On Tue, Nov 14, 2017 at 02:45:08PM +0100, Pavel Hrdina wrote: > > > If there is no sound device configured for the guest we can disable the > > > audio output because hot-plugging sound devices isn't supported. > > > > Are you sure about that. While libvirt may not have wired up ability to > > hotplug sound devices, I'm pretty sure that QEMU is able to hotplug > > them. > > > > Ff libvirt forceably disables the audio backend, now, and then future > > libvirt enables the pre-existing QEMU support for hotplug, existing VMs > > will be doomed. > > > > IOW, I don't think this patch is desirable. > > Right, I meant from libvirt POV. Anyway, if we allow to hot-plug sound > device, you have no control where the audio output will be connected. > Configuration of audio output is really stupid in QEMU. You need to use > environment variable, you have only one so you cannot configure > different sound devices to have different audio output and from > documentation it is not clear what is the default if you don't set the > QEMU_AUDIO_DRV. Checking the code gives you headache :). > > The default audio output depends on what audio drivers are enabled and > compiled in QEMU and on the order of the audio drivers, these can be > used as default: alsa, dsound, core, none, oss, pa, sdl, spice > (if there is spice graphics). > > So based on all of the findings, if we allow hot-plugging sound device > and the QEMU_AUDIO_DRV is not configured in advance there is no way how > you can configure the audio output and no way how we can tell which one > will be the default. > > I would rather have this limitation that you should start the domain > with sound device configured instead of allowing hot-plug since the > audio output design in QEMU is foobar. As mentioned in my reply to Peter, I don't think ability to configure the sound output is a blocker for hotplug, as there's valid reasons for hotplug whih don't involve multiple cards being present at once. Also 95+% of users of libvirt will have SPICE enabled, so all audio will get routed via SPICE regardless. So what would be most interesting there is not the ability to pick output backend, but the ability to pick which spice server instance is used. This is a more general task of wiring up multi-seat support that QEMU has had for a while, but libvirt doesn't yet support. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: explicitly disable audio if there is no sound device
On Tue, Nov 21, 2017 at 11:48:20 +, Daniel Berrange wrote: > On Tue, Nov 21, 2017 at 12:44:44PM +0100, Peter Krempa wrote: > > On Tue, Nov 21, 2017 at 11:24:06 +, Daniel Berrange wrote: > > > IOW, I don't think this patch is desirable. > > > > We could allow hotplug only if qemu will allow to specify the sound > > output per-soundcard which would avoid this problem. > > I agree that having ability to configure distinct outputs per sound card > might be useful, I don't think it is a blocking feature for hotplugging > sound cards. ie, there's no reason why a user should not be able to > unplug their current sound card, and plug in a new sound card for a > running guests - they would only ever have 1 sound card present at > a time in that scenario, so distinct outputs is not a requirement > for that usecase. While not a requirement, mandating that new features are available only with new software sometimes saves a lot of headaches. I would not mind if sound device hotplug will be supported only when specific outputs can be selected when adding the sound card and would not work without that. Unfortunately qemu does not yet support this and I don't think that they will any soon. Also code for outputting to pulseaudio does not get much love. I had to fix it so that the virtual sound mixer does not mess with physical michrophone volume setting in the host. And it was broken for years. I agree that this patch could bite us though, given that I don't see qemu fixing the sound backends soon as they did not do it until now. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: explicitly disable audio if there is no sound device
On Tue, Nov 21, 2017 at 11:24:06AM +, Daniel P. Berrange wrote: > On Tue, Nov 14, 2017 at 02:45:08PM +0100, Pavel Hrdina wrote: > > If there is no sound device configured for the guest we can disable the > > audio output because hot-plugging sound devices isn't supported. > > Are you sure about that. While libvirt may not have wired up ability to > hotplug sound devices, I'm pretty sure that QEMU is able to hotplug > them. > > Ff libvirt forceably disables the audio backend, now, and then future > libvirt enables the pre-existing QEMU support for hotplug, existing VMs > will be doomed. > > IOW, I don't think this patch is desirable. Right, I meant from libvirt POV. Anyway, if we allow to hot-plug sound device, you have no control where the audio output will be connected. Configuration of audio output is really stupid in QEMU. You need to use environment variable, you have only one so you cannot configure different sound devices to have different audio output and from documentation it is not clear what is the default if you don't set the QEMU_AUDIO_DRV. Checking the code gives you headache :). The default audio output depends on what audio drivers are enabled and compiled in QEMU and on the order of the audio drivers, these can be used as default: alsa, dsound, core, none, oss, pa, sdl, spice (if there is spice graphics). So based on all of the findings, if we allow hot-plugging sound device and the QEMU_AUDIO_DRV is not configured in advance there is no way how you can configure the audio output and no way how we can tell which one will be the default. I would rather have this limitation that you should start the domain with sound device configured instead of allowing hot-plug since the audio output design in QEMU is foobar. Pavel signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: explicitly disable audio if there is no sound device
On Tue, Nov 21, 2017 at 12:44:44PM +0100, Peter Krempa wrote: > On Tue, Nov 21, 2017 at 11:24:06 +, Daniel Berrange wrote: > > On Tue, Nov 14, 2017 at 02:45:08PM +0100, Pavel Hrdina wrote: > > > If there is no sound device configured for the guest we can disable the > > > audio output because hot-plugging sound devices isn't supported. > > > > Are you sure about that. While libvirt may not have wired up ability to > > hotplug sound devices, I'm pretty sure that QEMU is able to hotplug > > them. > > At least the USB sound card should allow hotplug in qemu, so I agree > with this... > > On the other hand the output should be a property which can be > configured individually for every sound card. I think it's desirable to > have a soundcard dedicated to one output method and a second one for a > different output method and let the guest OS decide on which cards the > sound will play. > > > Ff libvirt forceably disables the audio backend, now, and then future > > libvirt enables the pre-existing QEMU support for hotplug, existing VMs > > will be doomed. > > > > IOW, I don't think this patch is desirable. > > We could allow hotplug only if qemu will allow to specify the sound > output per-soundcard which would avoid this problem. I agree that having ability to configure distinct outputs per sound card might be useful, I don't think it is a blocking feature for hotplugging sound cards. ie, there's no reason why a user should not be able to unplug their current sound card, and plug in a new sound card for a running guests - they would only ever have 1 sound card present at a time in that scenario, so distinct outputs is not a requirement for that usecase. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: explicitly disable audio if there is no sound device
On Tue, Nov 21, 2017 at 11:24:06 +, Daniel Berrange wrote: > On Tue, Nov 14, 2017 at 02:45:08PM +0100, Pavel Hrdina wrote: > > If there is no sound device configured for the guest we can disable the > > audio output because hot-plugging sound devices isn't supported. > > Are you sure about that. While libvirt may not have wired up ability to > hotplug sound devices, I'm pretty sure that QEMU is able to hotplug > them. At least the USB sound card should allow hotplug in qemu, so I agree with this... On the other hand the output should be a property which can be configured individually for every sound card. I think it's desirable to have a soundcard dedicated to one output method and a second one for a different output method and let the guest OS decide on which cards the sound will play. > Ff libvirt forceably disables the audio backend, now, and then future > libvirt enables the pre-existing QEMU support for hotplug, existing VMs > will be doomed. > > IOW, I don't think this patch is desirable. We could allow hotplug only if qemu will allow to specify the sound output per-soundcard which would avoid this problem. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 16/21] resctrl: Add functions to work with resctrl allocations
On Tue, Nov 21, 2017 at 10:49:29AM +0100, Martin Kletzander wrote:
> On Wed, Nov 15, 2017 at 07:14:28PM +0100, Pavel Hrdina wrote:
> > On Mon, Nov 13, 2017 at 09:50:31AM +0100, Martin Kletzander wrote:
> > > With this commit we finally have a way to read and manipulate basic
> > > resctrl
> > > settings. Locking is done only on exposed functions that read/write
> > > from/to
> > > resctrlfs. Not in fuctions that are exposed in virresctrlpriv.h as those
> > > are
> > > only supposed to be used from tests.
> > >
> > > Signed-off-by: Martin Kletzander
> > > ---
> > > src/Makefile.am |2 +-
> > > src/libvirt_private.syms | 12 +
> > > src/util/virresctrl.c | 1012
> > > -
> > > src/util/virresctrl.h | 59 +++
> > > src/util/virresctrlpriv.h | 32 ++
> > > 5 files changed, 1115 insertions(+), 2 deletions(-)
> > > create mode 100644 src/util/virresctrlpriv.h
> > >
> > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > index 1d24231249de..ad113262fbb0 100644
> > > --- a/src/Makefile.am
> > > +++ b/src/Makefile.am
> > > @@ -167,7 +167,7 @@ UTIL_SOURCES = \
> > > util/virprocess.c util/virprocess.h \
> > > util/virqemu.c util/virqemu.h \
> > > util/virrandom.h util/virrandom.c \
> > > - util/virresctrl.h util/virresctrl.c \
> > > + util/virresctrl.h util/virresctrl.c util/virresctrlpriv.h
> > > \
> >
> > Use only single space instead of tab after "util/virresctrl.c" and
> > "util/virresctrlpriv.h".
> >
>
> That is actualy a single blank. It was a TAB that I didn't see in the
> code, but here, since it is one more character to the right, it shows.
> Again I don't see it in this reply as it again aligned differently :)
I opened the patch in vim and configured to display TAB and there are
two TABs, so please double check it in your editor :).
> [...]
>
> > > @@ -318,3 +465,866 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
> > > VIR_FREE(*controls);
> > > goto cleanup;
> > > }
> > > +
> > > +
> > > +/* Alloc-related functions */
> > > +static virResctrlAllocPerTypePtr
> > > +virResctrlAllocFindType(virResctrlAllocPtr *resctrl,
> > > +unsigned int level,
> > > +virCacheType type,
> > > +bool alloc)
> > > +{
> >
> > I don't like this implementation, it's too complex and it does two
> > different things based on a bool attribute. I see the benefit that
> > it's convenient but IMHO it's ugly.
> >
> > The only call path that doesn't need allocation is from
> > virResctrlAllocCheckCollision(). The remaining two calls
> > virResctrlAllocUpdateMask() and virResctrlAllocUpdateSize() needs
> > to allocate the internal structures of *virResctrlAllocPtr* object.
> >
>
> I'll duplicate the code if that's what's desired. I guess it will not
> look as gross as this then.
>
> > Another point is that there is no need to have this function create
> > new *virResctrlAllocPtr* object on demand, I would prefer creating
> > that object in advance before we even start filling all the data.
> >
>
> Just to make sure we are on the same page benefit-wise. There actually
> is. It will only be created if anyone adds size or mask to the
> allocation, otherwise it is NULL. It is easy to check that the
> allocation is empty. I'll redo it your way, but you need to have new
> object created, then update some stuff for it and then have a function
> that checks if the allocation is empty. And that needs three nested
> loops which there are too many already in this.
So the allocation happens in these public functions:
* virResctrlAllocNewFromInfo()
- There you know based on Info whether you need to allocate new
object or not. This is probably the only case where the automatic
allocation helps a little bit.
* virResctrlAllocSetSize()
- This is indirectly called from virDomainCachetuneDefParse() where
you know if there is any cache element to parse so you can
allocate new object if there is anything to set.
So there shouldn't be any need to have a function that will check the
allocation. Otherwise I wouldn't have suggested this modification.
>
> [...]
>
> > > +
> > > +static virBitmapPtr *
> > > +virResctrlAllocFindMask(virResctrlAllocPtr *resctrl,
> > > +unsigned int level,
> > > +virCacheType type,
> > > +unsigned int cache,
> > > +bool alloc)
> > > +{
> >
> > The code of this function can be merged into virResctrlAllocUpdateMask()
> > and again only allocate the structures and set the mask. Currently
> > there is no need for "Find" function if we will need it in the future
> > it should definitely only find the mask, not allocate it.
> >
>
> This is here just for separation. I can just cut-n-paste it into the
> other function. The same with other ones. It si
Re: [libvirt] [PATCH 3/6] qemu: explicitly disable audio if there is no sound device
On Tue, Nov 14, 2017 at 02:45:08PM +0100, Pavel Hrdina wrote: > If there is no sound device configured for the guest we can disable the > audio output because hot-plugging sound devices isn't supported. Are you sure about that. While libvirt may not have wired up ability to hotplug sound devices, I'm pretty sure that QEMU is able to hotplug them. Ff libvirt forceably disables the audio backend, now, and then future libvirt enables the pre-existing QEMU support for hotplug, existing VMs will be doomed. IOW, I don't think this patch is desirable. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: explicitly disable audio if there is no sound device
On Mon, Nov 20, 2017 at 05:55:10PM -0500, John Ferlan wrote:
>
>
> On 11/14/2017 08:45 AM, Pavel Hrdina wrote:
> > If there is no sound device configured for the guest we can disable the
> > audio output because hot-plugging sound devices isn't supported.
> >
> > Signed-off-by: Pavel Hrdina
> > ---
> > src/qemu/qemu_command.c | 5
> > +
> > tests/qemuxml2argvdata/qemuxml2argv-channel-spicevmc-old.args| 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-channel-spicevmc.args| 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-sdl-fullscreen.args | 1 +
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-sdl.args| 1 +
> > .../qemuxml2argv-graphics-spice-agent-file-xfer.args | 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-agentmouse.args | 2 +-
> > .../qemuxml2argv-graphics-spice-auto-socket-cfg.args | 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-auto-socket.args | 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args | 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.args | 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args | 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-sasl.args | 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-socket.args | 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.args| 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args | 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.args| 2 +-
> > tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args | 2 +-
> > 19 files changed, 23 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index e1ef1b05fa..c5c7bd7e54 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4447,6 +4447,11 @@ qemuBuildSoundAudioEnv(virCommandPtr cmd,
> > const virDomainDef *def,
> > virQEMUDriverConfigPtr cfg)
> > {
> > +if (def->nsounds == 0) {
> > +virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
> > +return;
> > +}
> > +
>
> But based on the changes to the .args file for Spice - wouldn't the
> default be whatever Spice had? Now we're requiring someone to configure
> the sound for Spice?
>
> What about a migration... On hostA with 3.9.0 - we have sound... We
> migrate to hostB with these patches and the sound goes away?
>
>
> > if (def->ngraphics == 0) {
> > if (cfg->nogfxAllowHostAudio)
>
> Also if there was no graphics and no sound device previously, the domain
> would be started with whatever QEMU_AUDIO_DRV was set to (outside
> libvirt context), with this path, right? So in this case, we then also
> would "lose" the sound - I think that'd be the text console case.
>
> Maybe I just need to be convince more on this one. Always "of concern"
> to remove some default just in case "someone" has assumed that [and I
> haven't looked ahead yet, so my opinion could change again ;-)]
If there is no sound device you have no audio even if you have graphic
device configured. It's the same as in real world, if you don't have
sound card you don't have audio output. So there is no issue with
migration because there was no audio output on hostA.
This is what is wrong with the current implementation, the audio output
is based on graphic device but there is no connection to sound device.
The only connection is that you can configure SPICE audio output
and the audio will be send to client via SPICE protocol.
So no matter what the QEMU_AUDIO_DRV is set to, if there is no sound
device there is no audio.
Pavel
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 21/21] docs: Add CAT (resctrl) support into news.xml
On Wed, Nov 15, 2017 at 05:22:24PM -0500, John Ferlan wrote: On 11/13/2017 03:50 AM, Martin Kletzander wrote: Signed-off-by: Martin Kletzander --- docs/news.xml | 11 +++ 1 file changed, 11 insertions(+) > diff --git a/docs/news.xml b/docs/news.xml index ef855d895886..e06c981e36da 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -35,6 +35,17 @@ + + + Added support for CAT (resctrl tuning) Actually Cache Allocation Technology, right? I don't know why I wanted to push resctrl in somehow. Fixed. + + + Domain vCPU threads can now have allocated some parts of host cache + using cachetune element in cputune. The using the + support is limited in some ways, but in the future it could be + extended. Instead of "in some ways, but..." - let's just describe the basics that got implemented. The ability to define for vCPU(s) how CAT will be configured for the domain for the cache id, level, and size. I left that part out and documented what's supported in, well, the documentation. There is no need to have this part in news.xml, I guess. Anyway, there'll be everything in v2 later. I trust you can come up with something more complete without writing too much. Reviewed-by: John Ferlan John + + signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 16/21] resctrl: Add functions to work with resctrl allocations
On Wed, Nov 15, 2017 at 07:14:28PM +0100, Pavel Hrdina wrote:
On Mon, Nov 13, 2017 at 09:50:31AM +0100, Martin Kletzander wrote:
With this commit we finally have a way to read and manipulate basic resctrl
settings. Locking is done only on exposed functions that read/write from/to
resctrlfs. Not in fuctions that are exposed in virresctrlpriv.h as those are
only supposed to be used from tests.
Signed-off-by: Martin Kletzander
---
src/Makefile.am |2 +-
src/libvirt_private.syms | 12 +
src/util/virresctrl.c | 1012 -
src/util/virresctrl.h | 59 +++
src/util/virresctrlpriv.h | 32 ++
5 files changed, 1115 insertions(+), 2 deletions(-)
create mode 100644 src/util/virresctrlpriv.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 1d24231249de..ad113262fbb0 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -167,7 +167,7 @@ UTIL_SOURCES = \
util/virprocess.c util/virprocess.h \
util/virqemu.c util/virqemu.h \
util/virrandom.h util/virrandom.c \
- util/virresctrl.h util/virresctrl.c \
+ util/virresctrl.h util/virresctrl.c util/virresctrlpriv.h
\
Use only single space instead of tab after "util/virresctrl.c" and
"util/virresctrlpriv.h".
That is actualy a single blank. It was a TAB that I didn't see in the
code, but here, since it is one more character to the right, it shows.
Again I don't see it in this reply as it again aligned differently :)
[...]
@@ -318,3 +465,866 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
VIR_FREE(*controls);
goto cleanup;
}
+
+
+/* Alloc-related functions */
+static virResctrlAllocPerTypePtr
+virResctrlAllocFindType(virResctrlAllocPtr *resctrl,
+unsigned int level,
+virCacheType type,
+bool alloc)
+{
I don't like this implementation, it's too complex and it does two
different things based on a bool attribute. I see the benefit that
it's convenient but IMHO it's ugly.
The only call path that doesn't need allocation is from
virResctrlAllocCheckCollision(). The remaining two calls
virResctrlAllocUpdateMask() and virResctrlAllocUpdateSize() needs
to allocate the internal structures of *virResctrlAllocPtr* object.
I'll duplicate the code if that's what's desired. I guess it will not
look as gross as this then.
Another point is that there is no need to have this function create
new *virResctrlAllocPtr* object on demand, I would prefer creating
that object in advance before we even start filling all the data.
Just to make sure we are on the same page benefit-wise. There actually
is. It will only be created if anyone adds size or mask to the
allocation, otherwise it is NULL. It is easy to check that the
allocation is empty. I'll redo it your way, but you need to have new
object created, then update some stuff for it and then have a function
that checks if the allocation is empty. And that needs three nested
loops which there are too many already in this.
[...]
+
+static virBitmapPtr *
+virResctrlAllocFindMask(virResctrlAllocPtr *resctrl,
+unsigned int level,
+virCacheType type,
+unsigned int cache,
+bool alloc)
+{
The code of this function can be merged into virResctrlAllocUpdateMask()
and again only allocate the structures and set the mask. Currently
there is no need for "Find" function if we will need it in the future
it should definitely only find the mask, not allocate it.
This is here just for separation. I can just cut-n-paste it into the
other function. The same with other ones. It sill just create bigger
functions that are IMNSHO less readable. Sure I can do that.
[...]
+int
+virResctrlAllocSetID(virResctrlAllocPtr alloc,
+ const char *id)
+{
+if (!id) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Resctrl allocation id cannot be NULL"));
+return -1;
+}
+
+return VIR_STRDUP(alloc->id, id);
+}
This is how I would expect all the other public functions to look like.
Simple, does one thing and there is no magic.
Well, because it does totally nothing. If all "public" functions would
do this then there would be no functionality =D
[...]
+static int
+virResctrlAllocParse(virResctrlAllocPtr *alloc,
+ const char *schemata)
+{
The virResctrlAllocPtr object should already exists and this function
should only parse the data into existing object.
Same as above, but OK, I want to get rid of this patchset, finally.
[...]
+int
+virResctrlAllocMasksAssign(virResctrlInfoPtr r_info,
+ virResctrlAllocPtr alloc,
+ void **save_ptr)
+{
+int ret = -1;
+unsigned int level = 0;
+virResctrlAllocPtr alloc_free = NULL;
+
+if (!r_info) {
+
Re: [libvirt] [PATCH 1/3] vsh: Make self-test more robust
On Thu, Nov 16, 2017 at 02:49:27PM +0100, Michal Privoznik wrote: > There are couple of limitations when it comes to option types and > flags for the options. For instance, VSH_OT_STRING cannot have > VSH_OFLAG_REQ set (commit c7543a728). For some reason this is > checked in vshCmddefHelp() but not in vshCmddefCheckInternals(). > > Signed-off-by: Michal Privoznik > --- [...] > -if (opt->type == VSH_OT_ARGV && cmd->opts[i + 1].name) > -return -1; /* argv option must be listed last */ > +break; > +case VSH_OT_ARGV: > +if (cmd->opts[i + 1].name) > +return -1; /* argv option must be listed last */ > +break; > + > +case VSH_OT_DATA: > +if (!(opt->flags & VSH_OFLAG_REQ)) > +return -1; /* OT_DATA should always be required. */ This got me thinking a bit, since we're going to do the checking here, is there a need for performing the same check within vshCmddefHelp too? My reasoning is that virsh-self-test is part of the test suite run at make check. Not a deal breaker though, just thinking out loud. > +break; > + > +case VSH_OT_INT: > +/* nada */ please don't... > +break; > +} Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] qemu: move QEMU_AUDIO_DRIVER out of graphic into sound
On Mon, Nov 20, 2017 at 05:39:38PM -0500, John Ferlan wrote:
>
>
> On 11/14/2017 08:45 AM, Pavel Hrdina wrote:
> > Setting the default audio output depends on specific graphic device
> > but requires having sound device configured as well and it's the sound
> > device that handles the audio.
> >
> > Signed-off-by: Pavel Hrdina
> > ---
> > src/qemu/qemu_command.c| 84
> > +++---
> > .../qemuxml2argv-clock-france.args | 2 +-
> > 2 files changed, 58 insertions(+), 28 deletions(-)
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index eb72db33ba..e1ef1b05fa 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4442,10 +4442,62 @@ qemuBuildSoundCodecStr(virDomainSoundDefPtr sound,
> > }
> > >
> > +static void
> > +qemuBuildSoundAudioEnv(virCommandPtr cmd,
> > + const virDomainDef *def,
> > + virQEMUDriverConfigPtr cfg)
> > +{
> > +if (def->ngraphics == 0) {
> > +if (cfg->nogfxAllowHostAudio)
> > +virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
> > +else
> > +virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
> > +} else {
> > +switch (def->graphics[def->ngraphics - 1]->type) {
>
> So it's the "last" graphics device that then defines "how" this all
> works? Makes sense for QEMU_AUDIO_DRV since whichever is last would be
> the winner and get the chicken dinner, but...
>
> > +case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
> > +/* If using SDL for video, then we should just let it
> > + * use QEMU's host audio drivers, possibly SDL too
> > + * User can set these two before starting libvirtd
> > + */
> > +virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
> > +virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL);
>
> ... if there was more than one graphics device defined, where one was
> SDL and it wasn't the last one, then theoretically at least this would
> not be defined.
This is intentional, I should have mentioned it in the commit message.
The original design was just wrong, nothing else. Setting
"SDL_AUDIODRIVER" if the SDL audio output is not used is pointless
and we shouldn't do it. I can move this change to separate patch.
Pavel
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] tests: add test cases for default sound output
On Mon, Nov 20, 2017 at 05:22:48PM -0500, John Ferlan wrote: > > > On 11/14/2017 08:45 AM, Pavel Hrdina wrote: > > These test cases models current situation where there is no way how > > to specify sound output and that it's based on which graphic device > > is the last one. > > > > Personally had a hard time parsing what the commit message said, but > based on what happens 2 patches from now, I think you're just trying to > add vm's w/ a sound device. Not so sure it's the "default" sound device > or just "a" sound device. Not sure how much clearer you could make > things or if it really matters - I'll leave it up to you to reread and > be sure what's happening is what you expect! Well I'm sure that this patch does what I expect from it :). How about this commit message: These test cases models current situation where there is no way how to specify audio output of sound devices. The audio output is currently based on which graphic device is configured as the last one in domain XML. Pavel signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
