5:57 a.m.
An alternative to
https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html
1462942877-31516-1-git-send-email-jfehlig(a)suse.com
This series adds a call to DeviceDefPostParse for the implicit video
which is being in the DomainDefPostParse phase, after DeviceDefPostParse
has been done on all the existing devices.
Ján Tomko (3):
Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse
Call per-device post-parse callback even on implicit video
Fill out default vram in DeviceDefPostParse
src/conf/domain_conf.c | 105 ++++++++++++++++++++++--------------------
src/qemu/qemu_parse_command.c | 2 -
2 files changed, 56 insertions(+), 51 deletions(-)
--
2.7.3
5:57 a.m.
New subject: [libvirt] [PATCH 1/3] Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse
Future commit will call DeviceDefPostParse on a device auto-added
in DomainDefPostParse.
---
src/conf/domain_conf.c | 78 +++++++++++++++++++++++++-------------------------
1 file changed, 39 insertions(+), 39 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 571b7bf..13d0f9f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3924,45 +3924,6 @@ virDomainDefPostParseTimer(virDomainDefPtr def)
}
-static int
-virDomainDefPostParseInternal(virDomainDefPtr def,
- virCapsPtr caps ATTRIBUTE_UNUSED,
- unsigned int parseFlags)
-{
- /* verify init path for container based domains */
- if (def->os.type == VIR_DOMAIN_OSTYPE_EXE && !def->os.init) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("init binary must be specified"));
- return -1;
- }
-
- if (virDomainDefPostParseMemory(def, parseFlags) < 0)
- return -1;
-
- if (virDomainDefRejectDuplicateControllers(def) < 0)
- return -1;
-
- if (virDomainDefRejectDuplicatePanics(def) < 0)
- return -1;
-
- if (virDomainDefPostParseTimer(def) < 0)
- return -1;
-
- if (virDomainDefAddImplicitDevices(def) < 0)
- return -1;
-
- /* Mark the first video as primary. If the user specified primary="yes",
- * the parser already inserted the device at def->videos[0] */
- if (def->nvideos != 0)
- def->videos[0]->primary = true;
-
- /* clean up possibly duplicated metadata entries */
- virDomainDefMetadataSanitize(def);
-
- return 0;
-}
-
-
/* Check if a drive type address $controller:$bus:$target:$unit is already
* taken by a disk or not.
*/
@@ -4399,6 +4360,45 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def
ATTRIBUTE_UNUSED,
}
+static int
+virDomainDefPostParseInternal(virDomainDefPtr def,
+ virCapsPtr caps ATTRIBUTE_UNUSED,
+ unsigned int parseFlags)
+{
+ /* verify init path for container based domains */
+ if (def->os.type == VIR_DOMAIN_OSTYPE_EXE && !def->os.init) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("init binary must be specified"));
+ return -1;
+ }
+
+ if (virDomainDefPostParseMemory(def, parseFlags) < 0)
+ return -1;
+
+ if (virDomainDefRejectDuplicateControllers(def) < 0)
+ return -1;
+
+ if (virDomainDefRejectDuplicatePanics(def) < 0)
+ return -1;
+
+ if (virDomainDefPostParseTimer(def) < 0)
+ return -1;
+
+ if (virDomainDefAddImplicitDevices(def) < 0)
+ return -1;
+
+ /* Mark the first video as primary. If the user specified primary="yes",
+ * the parser already inserted the device at def->videos[0] */
+ if (def->nvideos != 0)
+ def->videos[0]->primary = true;
+
+ /* clean up possibly duplicated metadata entries */
+ virDomainDefMetadataSanitize(def);
+
+ return 0;
+}
+
+
int
virDomainDefPostParse(virDomainDefPtr def,
virCapsPtr caps,
--
2.7.3
5:57 a.m.
New subject: [libvirt] [PATCH 2/3] Call per-device post-parse callback even on implicit video
Commit 6879be48 moved adding of an implicit video device after XML
parsing. As a result, libxlDomainDeviceDefPostParse() is no longer
called to set the default vram when adding an implicit device.
Commit 6879be48 assumes virDomainVideoDefaultRAM() will set the
default vram, but it returns 0 if the domain virtType is
VIR_DOMAIN_VIRT_XEN. Attempting to start an HVM domain with vram=0
results in
error: unsupported configuration: videoram must be at least 4MB for CIRRUS
The default vram setting for Xen HVM domains depends on the device
model used (qemu-xen vs qemu-traditional), hence setting the
default is deferred to libxlDomainDeviceDefPostParse().
Call the device post-parse callback even for implicit video,
to fill out the default vram even for VIR_DOMAIN_VIRT_XEN.
https://bugzilla.redhat.com/show_bug.cgi?id=1334557
Most-of-commit-message-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/conf/domain_conf.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 13d0f9f..b3165fc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4362,8 +4362,7 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def
ATTRIBUTE_UNUSED,
static int
virDomainDefPostParseInternal(virDomainDefPtr def,
- virCapsPtr caps ATTRIBUTE_UNUSED,
- unsigned int parseFlags)
+ struct virDomainDefPostParseDeviceIteratorData *data)
{
/* verify init path for container based domains */
if (def->os.type == VIR_DOMAIN_OSTYPE_EXE && !def->os.init) {
@@ -4372,7 +4371,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
return -1;
}
- if (virDomainDefPostParseMemory(def, parseFlags) < 0)
+ if (virDomainDefPostParseMemory(def, data->parseFlags) < 0)
return -1;
if (virDomainDefRejectDuplicateControllers(def) < 0)
@@ -4387,11 +4386,22 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
if (virDomainDefAddImplicitDevices(def) < 0)
return -1;
- /* Mark the first video as primary. If the user specified primary="yes",
- * the parser already inserted the device at def->videos[0] */
- if (def->nvideos != 0)
+ if (def->nvideos != 0) {
+ virDomainDeviceDef device = {
+ .type = VIR_DOMAIN_DEVICE_VIDEO,
+ .data.video = def->videos[0],
+ };
+
+ /* Mark the first video as primary. If the user specified
primary="yes",
+ * the parser already inserted the device at def->videos[0] */
def->videos[0]->primary = true;
+ /* videos[0] might have been added in AddImplicitDevices, after we've
+ * done the per-device post-parse */
+ if (virDomainDefPostParseDeviceIterator(NULL, &device, NULL, data) < 0)
+ return -1;
+ }
+
/* clean up possibly duplicated metadata entries */
virDomainDefMetadataSanitize(def);
@@ -4429,7 +4439,7 @@ virDomainDefPostParse(virDomainDefPtr def,
return ret;
- if ((ret = virDomainDefPostParseInternal(def, caps, parseFlags)) < 0)
+ if ((ret = virDomainDefPostParseInternal(def, &data)) < 0)
return ret;
if (virDomainDefPostParseCheckFeatures(def, xmlopt) < 0)
--
2.7.3
5:57 a.m.
New subject: [libvirt] [PATCH 3/3] Fill out default vram in DeviceDefPostParse
Move filling out the default video (v)ram to DeviceDefPostParse.
This means it can be removed from virDomainVideoDefParseXML
and qemuParseCommandLine. Also, we no longer need to special case
VIR_DOMAIN_VIRT_XEN, since the per-driver callback gets called
before the generic one.
---
src/conf/domain_conf.c | 15 ++++++---------
src/qemu/qemu_parse_command.c | 2 --
2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b3165fc..ed0c471 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4174,6 +4174,12 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) {
virDomainVideoDefPtr video = dev->data.video;
+ /* Fill out (V)RAM if the driver-specific callback did not do so */
+ if (video->ram == 0 && video->type == VIR_DOMAIN_VIDEO_TYPE_QXL)
+ video->ram = virDomainVideoDefaultRAM(def, video->type);
+ if (video->vram == 0)
+ video->vram = virDomainVideoDefaultRAM(def, video->type);
+
video->ram = VIR_ROUND_UP_POWER_OF_TWO(video->ram);
video->vram = VIR_ROUND_UP_POWER_OF_TWO(video->vram);
}
@@ -12045,10 +12051,6 @@ unsigned int
virDomainVideoDefaultRAM(const virDomainDef *def,
const virDomainVideoType type)
{
- /* Defer setting default vram to the Xen drivers */
- if (def->virtType == VIR_DOMAIN_VIRT_XEN)
- return 0;
-
switch (type) {
case VIR_DOMAIN_VIDEO_TYPE_VGA:
case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
@@ -12227,8 +12229,6 @@ virDomainVideoDefParseXML(xmlNodePtr node,
_("cannot parse video ram '%s'"), ram);
goto error;
}
- } else if (def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {
- def->ram = virDomainVideoDefaultRAM(dom, def->type);
}
if (vram) {
@@ -12237,8 +12237,6 @@ virDomainVideoDefParseXML(xmlNodePtr node,
_("cannot parse video vram '%s'"), vram);
goto error;
}
- } else {
- def->vram = virDomainVideoDefaultRAM(dom, def->type);
}
if (vram64) {
@@ -18697,7 +18695,6 @@ virDomainDefAddImplicitVideo(virDomainDefPtr def)
_("cannot determine default video type"));
goto cleanup;
}
- video->vram = virDomainVideoDefaultRAM(def, video->type);
video->heads = 1;
if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, video) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index 7ce90f9..e30586f 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -2583,9 +2583,7 @@ qemuParseCommandLine(virCapsPtr caps,
vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN;
else
vid->type = video;
- vid->vram = virDomainVideoDefaultRAM(def, vid->type);
if (vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {
- vid->ram = virDomainVideoDefaultRAM(def, vid->type);
vid->vgamem = QEMU_QXL_VGAMEM_DEFAULT;
} else {
vid->ram = 0;
--
2.7.3
4:47 p.m.
On 05/11/2016 04:57 AM, Ján Tomko wrote:
An alternative to
https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html
1462942877-31516-1-git-send-email-jfehlig(a)suse.com
This series adds a call to DeviceDefPostParse for the implicit video
which is being in the DomainDefPostParse phase, after DeviceDefPostParse
has been done on all the existing devices.
I much prefer this approach over fixing it in the driver, but I'm not terribly
familiar with this code, hence the attempt to fix it in libxl.
Ján Tomko (3):
Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse
Call per-device post-parse callback even on implicit video
Fill out default vram in DeviceDefPostParse
I reviewed and tested the patches and did not notice any problems - ACK series.
Regards,
Jim
3:30 a.m.
On Wed, May 11, 2016 at 03:47:06PM -0600, Jim Fehlig wrote:
On 05/11/2016 04:57 AM, Ján Tomko wrote:
> An alternative to
> https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html
> 1462942877-31516-1-git-send-email-jfehlig(a)suse.com
>
> This series adds a call to DeviceDefPostParse for the implicit video
> which is being in the DomainDefPostParse phase, after DeviceDefPostParse
> has been done on all the existing devices.
I much prefer this approach over fixing it in the driver, but I'm not terribly
familiar with this code, hence the attempt to fix it in libxl.
>
> Ján Tomko (3):
> Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse
> Call per-device post-parse callback even on implicit video
> Fill out default vram in DeviceDefPostParse
I reviewed and tested the patches and did not notice any problems - ACK series.
Thanks, pushed now.
Jan
6:34 a.m.
On Thu, May 12, 2016 at 10:30:24 +0200, Ján Tomko wrote:
On Wed, May 11, 2016 at 03:47:06PM -0600, Jim Fehlig wrote:
> On 05/11/2016 04:57 AM, Ján Tomko wrote:
> > An alternative to
> > https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html
> > 1462942877-31516-1-git-send-email-jfehlig(a)suse.com
> >
> > This series adds a call to DeviceDefPostParse for the implicit video
> > which is being in the DomainDefPostParse phase, after DeviceDefPostParse
> > has been done on all the existing devices.
>
> I much prefer this approach over fixing it in the driver, but I'm not terribly
> familiar with this code, hence the attempt to fix it in libxl.
>
> >
> > Ján Tomko (3):
> > Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse
> > Call per-device post-parse callback even on implicit video
> > Fill out default vram in DeviceDefPostParse
>
> I reviewed and tested the patches and did not notice any problems - ACK series.
>
Thanks, pushed now.
This broke few parts of the xen testsuite:
TEST: xmconfigtest
.!.!.!.!.....!.!.!.!.!.!.!.!.!.!.!.!.!.! 40
.!.!.!.!.!.!.!.!!.!.!.! 63 FAIL
TEST: xlconfigtest
...!!!.!.!.!.!.....!.!.!!! 26 FAIL
TEST: sexpr2xmltest
.!.!!!!!.........!....!..!!!!.!!!!!!!!!! 40
!!!!!!!!.! 50 FAIL
Some examples:
59) Xen XM-2-XML Format escape-paths ...
In '/home/pipo/libvirt/tests/xmconfigdata/test-escape-paths.xml':
Offset 1942
Expect [h]
Actual [vram='16384' h]
4:47 a.m.
On 05/12/2016 09:30 AM, Ján Tomko wrote:
On Wed, May 11, 2016 at 03:47:06PM -0600, Jim Fehlig wrote:
> On 05/11/2016 04:57 AM, Ján Tomko wrote:
>> An alternative to
>> https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html
>> 1462942877-31516-1-git-send-email-jfehlig(a)suse.com
>>
>> This series adds a call to DeviceDefPostParse for the implicit video
>> which is being in the DomainDefPostParse phase, after DeviceDefPostParse
>> has been done on all the existing devices.
>
> I much prefer this approach over fixing it in the driver, but I'm not terribly
> familiar with this code, hence the attempt to fix it in libxl.
>
>>
>> Ján Tomko (3):
>> Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse
>> Call per-device post-parse callback even on implicit video
>> Fill out default vram in DeviceDefPostParse
>
> I reviewed and tested the patches and did not notice any problems - ACK series.
>
Thanks, pushed now.
I wonder if this series should be also on v1.3.3-maint branch
alongside commit
96b21fb ("Fix tests to include video ram size") and Jim's series (400e716
b90c4b5
"libxl: A few more fixes related to vram") ?
The bug https://bugzilla.redhat.com/show_bug.cgi?id=1334557 was originally reported
on v1.3.3.1 so probably requires theses fixes too.
Joao
8:32 a.m.
On 05/16/2016 05:47 AM, Joao Martins wrote:
On 05/12/2016 09:30 AM, Ján Tomko wrote:
> On Wed, May 11, 2016 at 03:47:06PM -0600, Jim Fehlig wrote:
>> On 05/11/2016 04:57 AM, Ján Tomko wrote:
>>> An alternative to
>>> https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html
>>> 1462942877-31516-1-git-send-email-jfehlig(a)suse.com
>>>
>>> This series adds a call to DeviceDefPostParse for the implicit video
>>> which is being in the DomainDefPostParse phase, after DeviceDefPostParse
>>> has been done on all the existing devices.
>>
>> I much prefer this approach over fixing it in the driver, but I'm not
terribly
>> familiar with this code, hence the attempt to fix it in libxl.
>>
>>>
>>> Ján Tomko (3):
>>> Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse
>>> Call per-device post-parse callback even on implicit video
>>> Fill out default vram in DeviceDefPostParse
>>
>> I reviewed and tested the patches and did not notice any problems - ACK series.
>>
>
> Thanks, pushed now.
I wonder if this series should be also on v1.3.3-maint branch alongside commit
96b21fb ("Fix tests to include video ram size") and Jim's series (400e716
b90c4b5
"libxl: A few more fixes related to vram") ?
The bug https://bugzilla.redhat.com/show_bug.cgi?id=1334557 was originally reported
on v1.3.3.1 so probably requires theses fixes too.
I've pushed them to v1.3.3-maint now
Thanks,
Cole
9:32 a.m.
On 05/16/2016 09:32 AM, Cole Robinson wrote:
On 05/16/2016 05:47 AM, Joao Martins wrote:
> On 05/12/2016 09:30 AM, Ján Tomko wrote:
>> On Wed, May 11, 2016 at 03:47:06PM -0600, Jim Fehlig wrote:
>>> On 05/11/2016 04:57 AM, Ján Tomko wrote:
>>>> An alternative to
>>>> https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html
>>>> 1462942877-31516-1-git-send-email-jfehlig(a)suse.com
>>>>
>>>> This series adds a call to DeviceDefPostParse for the implicit video
>>>> which is being in the DomainDefPostParse phase, after DeviceDefPostParse
>>>> has been done on all the existing devices.
>>>
>>> I much prefer this approach over fixing it in the driver, but I'm not
terribly
>>> familiar with this code, hence the attempt to fix it in libxl.
>>>
>>>>
>>>> Ján Tomko (3):
>>>> Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse
>>>> Call per-device post-parse callback even on implicit video
>>>> Fill out default vram in DeviceDefPostParse
>>>
>>> I reviewed and tested the patches and did not notice any problems - ACK
series.
>>>
>>
>> Thanks, pushed now.
> I wonder if this series should be also on v1.3.3-maint branch alongside commit
> 96b21fb ("Fix tests to include video ram size") and Jim's series
(400e716 b90c4b5
> "libxl: A few more fixes related to vram") ?
>
> The bug https://bugzilla.redhat.com/show_bug.cgi?id=1334557 was originally reported
> on v1.3.3.1 so probably requires theses fixes too.
>
I've pushed them to v1.3.3-maint now
Also note there was another bug that made virt-install fail, something wrong
with the <graphics> XML output I suspect, but I didn't dig into it:
https://bugzilla.redhat.com/show_bug.cgi?id=1334562
Thanks,
Cole
3107
days inactive
3112
days old
9 comments
5 participants
participants (5)
-
Cole Robinson -
Jim Fehlig -
Joao Martins -
Ján Tomko -
Peter Krempa