diff mbox series

[1/8] qemu: parse: drop redundant video config

Message ID c8d266ffbed87948a174a6c36b04ecb142227708.1498675591.git.crobinso@redhat.com
State New
Headers show
Series qemu: Default to video type=virtio for machvirt | expand

Commit Message

Cole Robinson June 28, 2017, 6:49 p.m. UTC 
The ram/vram = 0 bits aren't needed, and PostParse will fill in the
needed QXL default

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 src/qemu/qemu_command.c       |  4 ----
 src/qemu/qemu_parse_command.c | 11 +----------
 2 files changed, 1 insertion(+), 14 deletions(-)

-- 
2.13.0

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

Comments

Cole Robinson July 11, 2017, 12:03 a.m. UTC | #1 
On 06/28/2017 02:49 PM, Cole Robinson wrote:
> The ram/vram = 0 bits aren't needed, and PostParse will fill in the

> needed QXL default

> 

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

>  src/qemu/qemu_command.c       |  4 ----

>  src/qemu/qemu_parse_command.c | 11 +----------

>  2 files changed, 1 insertion(+), 14 deletions(-)

> 

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c

> index c53ab97b9..ebf9c63bc 100644

> --- a/src/qemu/qemu_command.c

> +++ b/src/qemu/qemu_command.c

> @@ -7264,10 +7264,6 @@ qemuBuildObsoleteAccelArg(virCommandPtr cmd,

>          }

>          break;

>  

> -    case VIR_DOMAIN_VIRT_XEN:

> -        /* XXX better check for xenner */

> -        break;

> -

>      default:

>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,

>                         _("the QEMU binary does not support %s"),

> diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c

> index af9063c02..60c81f0ca 100644

> --- a/src/qemu/qemu_parse_command.c

> +++ b/src/qemu/qemu_parse_command.c

> @@ -2608,16 +2608,7 @@ qemuParseCommandLine(virCapsPtr caps,

>          virDomainVideoDefPtr vid;

>          if (VIR_ALLOC(vid) < 0)

>              goto error;

> -        if (def->virtType == VIR_DOMAIN_VIRT_XEN)

> -            vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN;

> -        else

> -            vid->type = video;

> -        if (vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {

> -            vid->vgamem = QEMU_QXL_VGAMEM_DEFAULT;

> -        } else {

> -            vid->ram = 0;

> -            vid->vgamem = 0;

> -        }

> +        vid->type = video;

>          vid->heads = 1;

>  

>          if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, vid) < 0) {

> 


Now that I look closer at this patch, I realize I accidentally squashed
together two changes: the XEN removal was supposed to be its own patch. I'll
respin after review comes in

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
John Ferlan July 18, 2017, 10:20 p.m. UTC | #2 
On 06/28/2017 02:49 PM, Cole Robinson wrote:
> The ram/vram = 0 bits aren't needed, and PostParse will fill in the

> needed QXL default

> 

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

>  src/qemu/qemu_command.c       |  4 ----

>  src/qemu/qemu_parse_command.c | 11 +----------

>  2 files changed, 1 insertion(+), 14 deletions(-)

> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c

> index c53ab97b9..ebf9c63bc 100644

> --- a/src/qemu/qemu_command.c

> +++ b/src/qemu/qemu_command.c

> @@ -7264,10 +7264,6 @@ qemuBuildObsoleteAccelArg(virCommandPtr cmd,

>          }

>          break;

>  

> -    case VIR_DOMAIN_VIRT_XEN:

> -        /* XXX better check for xenner */

> -        break;

> -

>      default:

>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,

>                         _("the QEMU binary does not support %s"),

> diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c

> index af9063c02..60c81f0ca 100644

> --- a/src/qemu/qemu_parse_command.c

> +++ b/src/qemu/qemu_parse_command.c

> @@ -2608,16 +2608,7 @@ qemuParseCommandLine(virCapsPtr caps,

>          virDomainVideoDefPtr vid;

>          if (VIR_ALLOC(vid) < 0)

>              goto error;

> -        if (def->virtType == VIR_DOMAIN_VIRT_XEN)

> -            vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN;


Did your follow-up comment mean to imply this would be done separately
too or just the qemu_command.c change?

I guess I see video default'ing to VIR_DOMAIN_VIDEO_TYPE_CIRRUS and
changing based on the presence of various qualifiers, but this seems to
force a specific type based on virtType for virtType == VIRT_XEN that
would seemingly be lost. What is actually on the command line I didn't
chase down...

Still the qemu_parse_command code isn't exactly highly used and I think
as long as it can be explained why there's not need to special case
VIRT_XEN any more perhaps in the commit message, then you've got my:

Reviewed-by: John Ferlan <jferlan@redhat.com>


John

> -        else

> -            vid->type = video;

> -        if (vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {

> -            vid->vgamem = QEMU_QXL_VGAMEM_DEFAULT;

> -        } else {

> -            vid->ram = 0;

> -            vid->vgamem = 0;

> -        }

> +        vid->type = video;

>          vid->heads = 1;

>  

>          if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, vid) < 0) {

> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Aug. 27, 2017, 1:45 p.m. UTC | #3 
On 07/18/2017 06:20 PM, John Ferlan wrote:
> 

> 

> On 06/28/2017 02:49 PM, Cole Robinson wrote:

>> The ram/vram = 0 bits aren't needed, and PostParse will fill in the

>> needed QXL default

>>

>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

>> ---

>>  src/qemu/qemu_command.c       |  4 ----

>>  src/qemu/qemu_parse_command.c | 11 +----------

>>  2 files changed, 1 insertion(+), 14 deletions(-)

>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c

>> index c53ab97b9..ebf9c63bc 100644

>> --- a/src/qemu/qemu_command.c

>> +++ b/src/qemu/qemu_command.c

>> @@ -7264,10 +7264,6 @@ qemuBuildObsoleteAccelArg(virCommandPtr cmd,

>>          }

>>          break;

>>  

>> -    case VIR_DOMAIN_VIRT_XEN:

>> -        /* XXX better check for xenner */

>> -        break;

>> -

>>      default:

>>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,

>>                         _("the QEMU binary does not support %s"),

>> diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c

>> index af9063c02..60c81f0ca 100644

>> --- a/src/qemu/qemu_parse_command.c

>> +++ b/src/qemu/qemu_parse_command.c

>> @@ -2608,16 +2608,7 @@ qemuParseCommandLine(virCapsPtr caps,

>>          virDomainVideoDefPtr vid;

>>          if (VIR_ALLOC(vid) < 0)

>>              goto error;

>> -        if (def->virtType == VIR_DOMAIN_VIRT_XEN)

>> -            vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN;

> 

> Did your follow-up comment mean to imply this would be done separately

> too or just the qemu_command.c change?

> 

> I guess I see video default'ing to VIR_DOMAIN_VIDEO_TYPE_CIRRUS and

> changing based on the presence of various qualifiers, but this seems to

> force a specific type based on virtType for virtType == VIRT_XEN that

> would seemingly be lost. What is actually on the command line I didn't

> chase down...

> 

> Still the qemu_parse_command code isn't exactly highly used and I think

> as long as it can be explained why there's not need to special case

> VIRT_XEN any more perhaps in the commit message, then you've got my:

> 

> Reviewed-by: John Ferlan <jferlan@redhat.com>

> 


Thanks for the review and sorry for the delayed response. I split out the xen
changes into their own commit with this message:

    qemu: Remove remnants of xenner support

    Both of these are dead code: qemu_command.c explicitly rejects
    VIRT_XEN earlier in the call chain, and qemu_parse_command.c
    will never set VIRT_XEN anymore

I pushed it along with patch 1, 2, and 4, since patch 3 is independent and has
some comments

Thanks,
Cole

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

Patch

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c53ab97b9..ebf9c63bc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7264,10 +7264,6 @@  qemuBuildObsoleteAccelArg(virCommandPtr cmd,
         }
         break;
 
-    case VIR_DOMAIN_VIRT_XEN:
-        /* XXX better check for xenner */
-        break;
-
     default:
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("the QEMU binary does not support %s"),
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index af9063c02..60c81f0ca 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -2608,16 +2608,7 @@  qemuParseCommandLine(virCapsPtr caps,
         virDomainVideoDefPtr vid;
         if (VIR_ALLOC(vid) < 0)
             goto error;
-        if (def->virtType == VIR_DOMAIN_VIRT_XEN)
-            vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN;
-        else
-            vid->type = video;
-        if (vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {
-            vid->vgamem = QEMU_QXL_VGAMEM_DEFAULT;
-        } else {
-            vid->ram = 0;
-            vid->vgamem = 0;
-        }
+        vid->type = video;
         vid->heads = 1;
 
         if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, vid) < 0) {