[libvirt] [PATCH] Clarify lack of generated IDE -device string in QEMU driver
The QEMU driver contained code to generate a -device string for piix4-ide, but wasn't using it. This was intentional. This change removes the string generation and adds a comment explaining why no -device is necessary. * src/qemu/qemu_conf.c: Remove VIR_DOMAIN_CONTROLLER_TYPE_IDE handler in qemuBuildControllerDevStr(). Add comments. --- src/qemu/qemu_conf.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index f4a6c08..1c4f326 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2121,11 +2121,8 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virBufferVSprintf(&buf, ",id=scsi%d", def->idx); break; + /* We always get an IDE controller, whether we want it or not. */ case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - virBufferAddLit(&buf, "piix4-ide"); - virBufferVSprintf(&buf, ",id=ide%d", def->idx); - break; - default: goto error; } @@ -3141,16 +3138,19 @@ int qemudBuildCommandLine(virConnectPtr conn, if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { for (i = 0 ; i < def->ncontrollers ; i++) { - char *scsi; - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + /* We don't add an explicit IDE controller because the provided + * PIIX4 device already includes one. It isn't possible to remove + * the PIIX4. */ + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) continue; ADD_ARG_LIT("-device"); - if (!(scsi = qemuBuildControllerDevStr(def->controllers[i]))) + char *devstr; + if (!(devstr = qemuBuildControllerDevStr(def->controllers[i]))) goto no_memory; - ADD_ARG(scsi); + ADD_ARG(devstr); } } -- 1.6.6
On Thu, Jan 28, 2010 at 10:35:48AM +0000, Matthew Booth wrote:
The QEMU driver contained code to generate a -device string for piix4-ide, but wasn't using it. This was intentional. This change removes the string generation and adds a comment explaining why no -device is necessary.
* src/qemu/qemu_conf.c: Remove VIR_DOMAIN_CONTROLLER_TYPE_IDE handler in qemuBuildControllerDevStr(). Add comments. --- src/qemu/qemu_conf.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index f4a6c08..1c4f326 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2121,11 +2121,8 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virBufferVSprintf(&buf, ",id=scsi%d", def->idx); break;
+ /* We always get an IDE controller, whether we want it or not. */ case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - virBufferAddLit(&buf, "piix4-ide"); - virBufferVSprintf(&buf, ",id=ide%d", def->idx); - break; - default: goto error; } @@ -3141,16 +3138,19 @@ int qemudBuildCommandLine(virConnectPtr conn,
if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { for (i = 0 ; i < def->ncontrollers ; i++) { - char *scsi; - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + /* We don't add an explicit IDE controller because the provided + * PIIX4 device already includes one. It isn't possible to remove + * the PIIX4. */ + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) continue;
That's not quite right because you changed the conditional to blacklist IDE insteasd of whitelisting SCSI. This means that FDC now falls through. We do not need to explicitly add the FDC, since that is also provided automatically on the ISA bridge that's behind the PIIX4.
ADD_ARG_LIT("-device");
- if (!(scsi = qemuBuildControllerDevStr(def->controllers[i]))) + char *devstr; + if (!(devstr = qemuBuildControllerDevStr(def->controllers[i]))) goto no_memory;
- ADD_ARG(scsi); + ADD_ARG(devstr); } }
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 28/01/10 10:57, Daniel P. Berrange wrote:
On Thu, Jan 28, 2010 at 10:35:48AM +0000, Matthew Booth wrote:
The QEMU driver contained code to generate a -device string for piix4-ide, but wasn't using it. This was intentional. This change removes the string generation and adds a comment explaining why no -device is necessary.
* src/qemu/qemu_conf.c: Remove VIR_DOMAIN_CONTROLLER_TYPE_IDE handler in qemuBuildControllerDevStr(). Add comments. --- src/qemu/qemu_conf.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index f4a6c08..1c4f326 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2121,11 +2121,8 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virBufferVSprintf(&buf, ",id=scsi%d", def->idx); break;
+ /* We always get an IDE controller, whether we want it or not. */ case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - virBufferAddLit(&buf, "piix4-ide"); - virBufferVSprintf(&buf, ",id=ide%d", def->idx); - break; - default: goto error; } @@ -3141,16 +3138,19 @@ int qemudBuildCommandLine(virConnectPtr conn,
if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { for (i = 0 ; i < def->ncontrollers ; i++) { - char *scsi; - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + /* We don't add an explicit IDE controller because the provided + * PIIX4 device already includes one. It isn't possible to remove + * the PIIX4. */ + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) continue;
That's not quite right because you changed the conditional to blacklist IDE insteasd of whitelisting SCSI. This means that FDC now falls through. We do not need to explicitly add the FDC, since that is also provided automatically on the ISA bridge that's behind the PIIX4.
Ok. Still think a blacklist is more appropriate, though. I'll add FDC to the blacklist. Are there any more? Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
The QEMU driver contained code to generate a -device string for piix4-ide, but wasn't using it. This change removes this string generation. It also adds a comment explaining why IDE and FDC controllers don't generate -device strings. The change also generates an error if a sata controller is specified for a QEMU domain, as this isn't supported. * src/qemu/qemu_conf.c: Remove VIR_DOMAIN_CONTROLLER_TYPE_IDE handler in qemuBuildControllerDevStr(). Ignore IDE and FDC controllers. Error if SATA controller is discovered. Add comments. --- src/qemu/qemu_conf.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index f4a6c08..1c4f326 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2121,11 +2121,8 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virBufferVSprintf(&buf, ",id=scsi%d", def->idx); break; + /* We always get an IDE controller, whether we want it or not. */ case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - virBufferAddLit(&buf, "piix4-ide"); - virBufferVSprintf(&buf, ",id=ide%d", def->idx); - break; - default: goto error; } @@ -3141,16 +3138,19 @@ int qemudBuildCommandLine(virConnectPtr conn, if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { for (i = 0 ; i < def->ncontrollers ; i++) { - char *scsi; - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + /* We don't add an explicit IDE controller because the provided + * PIIX4 device already includes one. It isn't possible to remove + * the PIIX4. */ + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) continue; ADD_ARG_LIT("-device"); - if (!(scsi = qemuBuildControllerDevStr(def->controllers[i]))) + char *devstr; + if (!(devstr = qemuBuildControllerDevStr(def->controllers[i]))) goto no_memory; - ADD_ARG(scsi); + ADD_ARG(devstr); } } -- 1.6.6
The QEMU driver contained code to generate a -device string for piix4-ide, but wasn't using it. This change removes this string generation. It also adds a comment explaining why IDE and FDC controllers don't generate -device strings. The change also generates an error if a sata controller is specified for a QEMU domain, as this isn't supported. * src/qemu/qemu_conf.c: Remove VIR_DOMAIN_CONTROLLER_TYPE_IDE handler in qemuBuildControllerDevStr(). Ignore IDE and FDC controllers. Error if SATA controller is discovered. Add comments. --- src/qemu/qemu_conf.c | 26 ++++++++++++++++++-------- 1 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index f4a6c08..3b7793f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2121,11 +2121,8 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virBufferVSprintf(&buf, ",id=scsi%d", def->idx); break; + /* We always get an IDE controller, whether we want it or not. */ case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - virBufferAddLit(&buf, "piix4-ide"); - virBufferVSprintf(&buf, ",id=ide%d", def->idx); - break; - default: goto error; } @@ -3141,16 +3138,29 @@ int qemudBuildCommandLine(virConnectPtr conn, if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { for (i = 0 ; i < def->ncontrollers ; i++) { - char *scsi; - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + virDomainControllerDefPtr cont = def->controllers[i]; + + /* We don't add an explicit IDE or FD controller because the + * provided PIIX4 device already includes one. It isn't possible to + * remove the PIIX4. */ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE || + cont->type == VIR_DOMAIN_CONTROLLER_TYPE_FDC) continue; + /* QEMU doesn't implement a SATA driver */ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) { + qemudReportError(conn, NULL, NULL, VIR_ERR_NO_SUPPORT, + "%s", _("SATA is not supported with this QEMU binary")); + goto error; + } + ADD_ARG_LIT("-device"); - if (!(scsi = qemuBuildControllerDevStr(def->controllers[i]))) + char *devstr; + if (!(devstr = qemuBuildControllerDevStr(def->controllers[i]))) goto no_memory; - ADD_ARG(scsi); + ADD_ARG(devstr); } } -- 1.6.6
On Thu, Jan 28, 2010 at 12:53:56PM +0000, Matthew Booth wrote:
The QEMU driver contained code to generate a -device string for piix4-ide, but wasn't using it. This change removes this string generation. It also adds a comment explaining why IDE and FDC controllers don't generate -device strings.
The change also generates an error if a sata controller is specified for a QEMU domain, as this isn't supported.
* src/qemu/qemu_conf.c: Remove VIR_DOMAIN_CONTROLLER_TYPE_IDE handler in qemuBuildControllerDevStr(). Ignore IDE and FDC controllers. Error if SATA controller is discovered. Add comments. --- src/qemu/qemu_conf.c | 26 ++++++++++++++++++-------- 1 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index f4a6c08..3b7793f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2121,11 +2121,8 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virBufferVSprintf(&buf, ",id=scsi%d", def->idx); break;
+ /* We always get an IDE controller, whether we want it or not. */ case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - virBufferAddLit(&buf, "piix4-ide"); - virBufferVSprintf(&buf, ",id=ide%d", def->idx); - break; - default: goto error; } @@ -3141,16 +3138,29 @@ int qemudBuildCommandLine(virConnectPtr conn,
if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { for (i = 0 ; i < def->ncontrollers ; i++) { - char *scsi; - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + virDomainControllerDefPtr cont = def->controllers[i]; + + /* We don't add an explicit IDE or FD controller because the + * provided PIIX4 device already includes one. It isn't possible to + * remove the PIIX4. */ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE || + cont->type == VIR_DOMAIN_CONTROLLER_TYPE_FDC) continue;
+ /* QEMU doesn't implement a SATA driver */ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) { + qemudReportError(conn, NULL, NULL, VIR_ERR_NO_SUPPORT, + "%s", _("SATA is not supported with this QEMU binary")); + goto error; + } + ADD_ARG_LIT("-device");
- if (!(scsi = qemuBuildControllerDevStr(def->controllers[i]))) + char *devstr; + if (!(devstr = qemuBuildControllerDevStr(def->controllers[i]))) goto no_memory;
- ADD_ARG(scsi); + ADD_ARG(devstr); } }
ACK, looks fine now Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Jan 28, 2010 at 12:53:56PM +0000, Matthew Booth wrote:
The QEMU driver contained code to generate a -device string for piix4-ide, but wasn't using it. This change removes this string generation. It also adds a comment explaining why IDE and FDC controllers don't generate -device strings.
The change also generates an error if a sata controller is specified for a QEMU domain, as this isn't supported.
* src/qemu/qemu_conf.c: Remove VIR_DOMAIN_CONTROLLER_TYPE_IDE handler in qemuBuildControllerDevStr(). Ignore IDE and FDC controllers. Error if SATA controller is discovered. Add comments. --- src/qemu/qemu_conf.c | 26 ++++++++++++++++++-------- 1 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index f4a6c08..3b7793f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2121,11 +2121,8 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virBufferVSprintf(&buf, ",id=scsi%d", def->idx); break;
+ /* We always get an IDE controller, whether we want it or not. */ case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - virBufferAddLit(&buf, "piix4-ide"); - virBufferVSprintf(&buf, ",id=ide%d", def->idx); - break; - default: goto error; } @@ -3141,16 +3138,29 @@ int qemudBuildCommandLine(virConnectPtr conn,
if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { for (i = 0 ; i < def->ncontrollers ; i++) { - char *scsi; - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + virDomainControllerDefPtr cont = def->controllers[i]; + + /* We don't add an explicit IDE or FD controller because the + * provided PIIX4 device already includes one. It isn't possible to + * remove the PIIX4. */ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE || + cont->type == VIR_DOMAIN_CONTROLLER_TYPE_FDC) continue;
+ /* QEMU doesn't implement a SATA driver */ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) { + qemudReportError(conn, NULL, NULL, VIR_ERR_NO_SUPPORT, + "%s", _("SATA is not supported with this QEMU binary")); + goto error; + } + ADD_ARG_LIT("-device");
- if (!(scsi = qemuBuildControllerDevStr(def->controllers[i]))) + char *devstr; + if (!(devstr = qemuBuildControllerDevStr(def->controllers[i]))) goto no_memory;
- ADD_ARG(scsi); + ADD_ARG(devstr); } }
Okay, pushed, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Matthew Booth