On Sat, Oct 08, 2016 at 10:01:14AM -0400, John Ferlan wrote:
>
>
> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> > src/qemu/qemu_command.c | 140 ++++++++++++++++++++++++++----------------------
> > 1 file changed, 76 insertions(+), 64 deletions(-)
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index d283c67..aef8c79 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4371,6 +4371,81 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
> >
> >
> > static int
> > +qemuBuildVgaVideoCommand(virCommandPtr cmd,
> > + const virDomainDef *def,
>
> Instead of *def here, you could have
>
> const virDomainVideoDefPtr video,
>
> and then replace all the "def->videos[0]" with video
>
> > + virQEMUCapsPtr qemuCaps)
> > +{
> > + const char *vgastr = qemuVideoTypeToString(def->videos[0]->type);
> > + if (!vgastr || STREQ(vgastr, "")) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("video type %s is not supported with QEMU"),
> > + virDomainVideoTypeToString(def->videos[0]->type));
> > + return -1;
> > + }
> > +
> > + virCommandAddArgList(cmd, "-vga", vgastr, NULL);
> > +
> > + /* If we cannot use --device option to specify the video device
> > + * in QEMU we will fallback to the old --vga option. To get the
> > + * correct device name for the --vga option the 'qemuVideo' is
> > + * used, but to set some device attributes we need to use the
> > + * --global option and for that we need to specify the device
> > + * name the same as for --device option and for that we need to
> > + * use 'qemuDeviceVideo'.
> > + *
> > + * See 'Graphics Devices' section in docs/qdev-device-use.txt in
> > + * QEMU repository.
> > + */
> > + const char *dev = qemuDeviceVideoTypeToString(def->videos[0]->type);
>
> While we're at moving things around - can the "const char *dev... " be
> defined at the top and not in the middle somewhere?
>
> > +
> > + if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
> > + (def->videos[0]->vram || def->videos[0]->ram)) {
> > + unsigned int ram = def->videos[0]->ram;
> > + unsigned int vram = def->videos[0]->vram;
> > + unsigned int vram64 = def->videos[0]->vram64;
> > + unsigned int vgamem = def->videos[0]->vgamem;
> > +
> > + if (ram) {
> > + virCommandAddArg(cmd, "-global");
> > + virCommandAddArgFormat(cmd, "%s.ram_size=%u",
> > + dev, ram * 1024);
> > + }
> > + if (vram) {
> > + virCommandAddArg(cmd, "-global");
> > + virCommandAddArgFormat(cmd, "%s.vram_size=%u",
> > + dev, vram * 1024);
> > + }
> > + if (vram64 &&
> > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VRAM64)) {
> > + virCommandAddArg(cmd, "-global");
> > + virCommandAddArgFormat(cmd, "%s.vram64_size_mb=%u",
> > + dev, vram64 / 1024);
> > + }
> > + if (vgamem &&
> > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGAMEM)) {
> > + virCommandAddArg(cmd, "-global");
> > + virCommandAddArgFormat(cmd, "%s.vgamem_mb=%u",
> > + dev, vgamem / 1024);
> > + }
> > + }
> > +
> > + if (def->videos[0]->vram &&
> > + ((def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
> > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
> > + (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
> > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VMWARE_SVGA_VGAMEM)))) {
> > + unsigned int vram = def->videos[0]->vram;
> > +
> > + virCommandAddArg(cmd, "-global");
> > + virCommandAddArgFormat(cmd, "%s.vgamem_mb=%u",
> > + dev, vram / 1024);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static int
> > qemuBuildVideoCommandLine(virCommandPtr cmd,
> > const virDomainDef *def,
> > virQEMUCapsPtr qemuCaps)
> > @@ -4398,71 +4473,8 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
> > if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) {
>
> I was going to suggest something regarding removing primaryVideoType,
> but I see the next patch does more overhaul...
>
> > /* nothing - vga has no effect on Xen pvfb */
> > } else {
> > - const char *vgastr = qemuVideoTypeToString(primaryVideoType);
> > - if (!vgastr || STREQ(vgastr, "")) {
> > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > - _("video type %s is not supported with QEMU"),
> > - virDomainVideoTypeToString(primaryVideoType));
> > + if (qemuBuildVgaVideoCommand(cmd, def, qemuCaps) < 0)
>
> To augment my comment above, pass def->videos[0] instead of def. It even
> works with the next patch.
>
> ACK in principle. I'm not requiring the passing of video, just well,
> strongly suggesting it... makes reading things oh so much easier.
Thanks, I'll fix it but in separate patch, I would like to leave this patch as
clean code movement.
Pavel
>
> John
>
> > return -1;
> > - }
> > -
> > - virCommandAddArgList(cmd, "-vga", vgastr, NULL);
> > -
> > - /* If we cannot use --device option to specify the video device
> > - * in QEMU we will fallback to the old --vga option. To get the
> > - * correct device name for the --vga option the 'qemuVideo' is
> > - * used, but to set some device attributes we need to use the
> > - * --global option and for that we need to specify the device
> > - * name the same as for --device option and for that we need to
> > - * use 'qemuDeviceVideo'.
> > - *
> > - * See 'Graphics Devices' section in docs/qdev-device-use.txt in
> > - * QEMU repository.
> > - */
> > - const char *dev = qemuDeviceVideoTypeToString(primaryVideoType);
> > -
> > - if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
> > - (def->videos[0]->vram || def->videos[0]->ram)) {
> > - unsigned int ram = def->videos[0]->ram;
> > - unsigned int vram = def->videos[0]->vram;
> > - unsigned int vram64 = def->videos[0]->vram64;
> > - unsigned int vgamem = def->videos[0]->vgamem;
> > -
> > - if (ram) {
> > - virCommandAddArg(cmd, "-global");
> > - virCommandAddArgFormat(cmd, "%s.ram_size=%u",
> > - dev, ram * 1024);
> > - }
> > - if (vram) {
> > - virCommandAddArg(cmd, "-global");
> > - virCommandAddArgFormat(cmd, "%s.vram_size=%u",
> > - dev, vram * 1024);
> > - }
> > - if (vram64 &&
> > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VRAM64)) {
> > - virCommandAddArg(cmd, "-global");
> > - virCommandAddArgFormat(cmd, "%s.vram64_size_mb=%u",
> > - dev, vram64 / 1024);
> > - }
> > - if (vgamem &&
> > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGAMEM)) {
> > - virCommandAddArg(cmd, "-global");
> > - virCommandAddArgFormat(cmd, "%s.vgamem_mb=%u",
> > - dev, vgamem / 1024);
> > - }
> > - }
> > -
> > - if (def->videos[0]->vram &&
> > - ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VGA &&
> > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
> > - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
> > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_VMWARE_SVGA_VGAMEM)))) {
> > - unsigned int vram = def->videos[0]->vram;
> > -
> > - virCommandAddArg(cmd, "-global");
> > - virCommandAddArgFormat(cmd, "%s.vgamem_mb=%u",
> > - dev, vram / 1024);
> > - }
> > }
> >
> > for (i = 1; i < def->nvideos; i++) {
> >
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list