[libvirt] [PATCH] qemuBuildDriveDevStr: Prefer default aliases for IDE bus

Michal Privoznik posted 1 patch 6 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/dce4a2acb89a427e68de52e77f4dbda4827c5644.1510231402.git.mprivozn@redhat.com
src/qemu/qemu_command.c                               | 14 +++++++++++---
tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args |  4 ++--
2 files changed, 13 insertions(+), 5 deletions(-)
[libvirt] [PATCH] qemuBuildDriveDevStr: Prefer default aliases for IDE bus
Posted by Michal Privoznik 6 years, 5 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1434451

When testing user aliases it was discovered that for 440fx
machine type which has default IDE bus builtin, domain cannot
start if IDE controller has the user provided alias. This is
because for 440fx we don't put the IDE controller onto the
command line (since it is builtin) and therefore any device that
is plugged onto the bus must use the default alias.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c                               | 14 +++++++++++---
 tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args |  4 ++--
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 364196783..6cc77df2e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1886,9 +1886,17 @@ qemuBuildDriveDevStr(const virDomainDef *def,
             virBufferAddLit(&opt, "ide-drive");
         }
 
-        if (!(contAlias = virDomainControllerAliasFind(def, VIR_DOMAIN_CONTROLLER_TYPE_IDE,
-                                                       disk->info.addr.drive.controller)))
-           goto error;
+        /* When domain has builtin IDE controller we don't put it onto cmd
+         * line. Therefore we can't set its alias. In that case, use the
+         * default one. */
+        if (qemuDomainHasBuiltinIDE(def)) {
+            contAlias = "ide";
+        } else {
+            if (!(contAlias = virDomainControllerAliasFind(def,
+                                                           VIR_DOMAIN_CONTROLLER_TYPE_IDE,
+                                                           disk->info.addr.drive.controller)))
+                goto error;
+        }
         virBufferAsprintf(&opt, ",bus=%s.%d,unit=%d",
                           contAlias,
                           disk->info.addr.drive.bus,
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args b/tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args
index 62fbd567b..1719c1bc8 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args
@@ -44,8 +44,8 @@ id=drive-ua-myEncryptedDisk1 \
 id=ua-myEncryptedDisk1 \
 -drive file=/home/zippy/tmp/install-amd64-minimal-20140619.iso,format=raw,\
 if=none,media=cdrom,id=drive-ua-WhatAnAwesomeCDROM,readonly=on,cache=none \
--device ide-drive,bus=ua-DoesAnybodyStillUseIDE.1,unit=0,\
-drive=drive-ua-WhatAnAwesomeCDROM,id=ua-WhatAnAwesomeCDROM \
+-device ide-drive,bus=ide.1,unit=0,drive=drive-ua-WhatAnAwesomeCDROM,\
+id=ua-WhatAnAwesomeCDROM \
 -device virtio-net-pci,vlan=0,id=ua-CheckoutThisNIC,mac=52:54:00:d6:c0:0b,\
 bus=pci.0,addr=0x3 \
 -net tap,fd=3,vlan=0,name=hostua-CheckoutThisNIC \
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildDriveDevStr: Prefer default aliases for IDE bus
Posted by Martin Kletzander 6 years, 5 months ago
On Thu, Nov 09, 2017 at 01:43:22PM +0100, Michal Privoznik wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1434451
>
>When testing user aliases it was discovered that for 440fx
>machine type which has default IDE bus builtin, domain cannot
>start if IDE controller has the user provided alias. This is
>because for 440fx we don't put the IDE controller onto the
>command line (since it is builtin) and therefore any device that
>is plugged onto the bus must use the default alias.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_command.c                               | 14 +++++++++++---
> tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args |  4 ++--
> 2 files changed, 13 insertions(+), 5 deletions(-)
>

Look like that's fine in this case.  ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildDriveDevStr: Prefer default aliases for IDE bus
Posted by Michal Privoznik 6 years, 5 months ago
On 11/10/2017 03:50 PM, Martin Kletzander wrote:
> On Thu, Nov 09, 2017 at 01:43:22PM +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
>>
>> When testing user aliases it was discovered that for 440fx
>> machine type which has default IDE bus builtin, domain cannot
>> start if IDE controller has the user provided alias. This is
>> because for 440fx we don't put the IDE controller onto the
>> command line (since it is builtin) and therefore any device that
>> is plugged onto the bus must use the default alias.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/qemu/qemu_command.c                               | 14 +++++++++++---
>> tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args |  4 ++--
>> 2 files changed, 13 insertions(+), 5 deletions(-)
>>
> 
> Look like that's fine in this case.  ACK.

Thanks, pushed.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildDriveDevStr: Prefer default aliases for IDE bus
Posted by Ján Tomko 6 years, 5 months ago
On Thu, Nov 09, 2017 at 01:43:22PM +0100, Michal Privoznik wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1434451
>
>When testing user aliases it was discovered that for 440fx
>machine type which has default IDE bus builtin, domain cannot
>start if IDE controller has the user provided alias. This is
>because for 440fx we don't put the IDE controller onto the
>command line (since it is builtin) and therefore any device that
>is plugged onto the bus must use the default alias.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_command.c                               | 14 +++++++++++---
> tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args |  4 ++--
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index 364196783..6cc77df2e 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -1886,9 +1886,17 @@ qemuBuildDriveDevStr(const virDomainDef *def,
>             virBufferAddLit(&opt, "ide-drive");
>         }
>
>-        if (!(contAlias = virDomainControllerAliasFind(def, VIR_DOMAIN_CONTROLLER_TYPE_IDE,
>-                                                       disk->info.addr.drive.controller)))
>-           goto error;
>+        /* When domain has builtin IDE controller we don't put it onto cmd
>+         * line. Therefore we can't set its alias. In that case, use the
>+         * default one. */
>+        if (qemuDomainHasBuiltinIDE(def)) {
>+            contAlias = "ide";

This logic would fit better inside virDomainControllerAliasFind.

Also, the implicit pci-root on i440fx could also use that treatment.

We do not format aliases for PS2 mouse and keyboard either, but I cannot
imagine why the qemu alias would need to match the libvirt alias in that
case.

Any other devices come to mind?

Jan

>+        } else {
>+            if (!(contAlias = virDomainControllerAliasFind(def,
>+                                                           VIR_DOMAIN_CONTROLLER_TYPE_IDE,
>+                                                           disk->info.addr.drive.controller)))
>+                goto error;
>+        }
>         virBufferAsprintf(&opt, ",bus=%s.%d,unit=%d",
>                           contAlias,
>                           disk->info.addr.drive.bus,
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildDriveDevStr: Prefer default aliases for IDE bus
Posted by Michal Privoznik 6 years, 5 months ago
On 11/10/2017 04:03 PM, Ján Tomko wrote:
> On Thu, Nov 09, 2017 at 01:43:22PM +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
>>
>> When testing user aliases it was discovered that for 440fx
>> machine type which has default IDE bus builtin, domain cannot
>> start if IDE controller has the user provided alias. This is
>> because for 440fx we don't put the IDE controller onto the
>> command line (since it is builtin) and therefore any device that
>> is plugged onto the bus must use the default alias.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/qemu/qemu_command.c                               | 14 +++++++++++---
>> tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args |  4 ++--
>> 2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 364196783..6cc77df2e 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1886,9 +1886,17 @@ qemuBuildDriveDevStr(const virDomainDef *def,
>>             virBufferAddLit(&opt, "ide-drive");
>>         }
>>
>> -        if (!(contAlias = virDomainControllerAliasFind(def,
>> VIR_DOMAIN_CONTROLLER_TYPE_IDE,
>> -                                                      
>> disk->info.addr.drive.controller)))
>> -           goto error;
>> +        /* When domain has builtin IDE controller we don't put it
>> onto cmd
>> +         * line. Therefore we can't set its alias. In that case, use the
>> +         * default one. */
>> +        if (qemuDomainHasBuiltinIDE(def)) {
>> +            contAlias = "ide";
> 
> This logic would fit better inside virDomainControllerAliasFind.

Would it? virDomain*() functions live in src/conf/ and as such should be
driver agnostic. What is builtin controller in one hv can be explicitly
configurable in another (or not exist at all). I'm failing to see reason
for this function to special case drivers. Regardless, it can't call
qemuDomain*(), can it?

> 
> Also, the implicit pci-root on i440fx could also use that treatment.

Okay.

> 
> We do not format aliases for PS2 mouse and keyboard either, but I cannot
> imagine why the qemu alias would need to match the libvirt alias in that
> case.

Yup.

> 
> Any other devices come to mind?

None so far.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list