Pritesh Kothari wrote:

Hi All,

There was a minor bug in selecting the graphics type. if the graphics type was desktop it was assumed that display is set for it, and thus crashed on strdup, so now checking if display is present before setting it.

The second bug was while setting the 3d acceleration parameter, VIR_ALLOC() returns 0 or greater if success and not the other way round.

Lastly added OOM checks in few places which were missed earlier. diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index edcb39b..885908c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -1838,13 +1838,16 @@ static char *vboxDomainDumpXML(virDomainPtr dom, int flags) { def->videos[0]->type = VIR_DOMAIN_VIDEO_TYPE_VBOX; def->videos[0]->vram = VRAMSize; def->videos[0]->heads = monitorCount; - if (VIR_ALLOC(def->videos[0]->accel)) { + if (VIR_ALLOC(def->videos[0]->accel) >= 0) { def->videos[0]->accel->support3d = accelerate3DEnabled; /* Not supported yet, but should be in 3.1 soon */ def->videos[0]->accel->support2d = 0; - } - } - } + } else + virReportOOMError(dom->conn); + } else + virReportOOMError(dom->conn); + } else + virReportOOMError(dom->conn); }

To be honest, I find this whole section of code difficult to read. The problem is that the "checking for success" on VIR_ALLOC() makes it so that the code you care about is always heavily indented, and leads to the triple else block you have above. I much prefer the style used in the rest of libvirt, where you check for failure and get out, leaving the code you care about not indented so heavily. The same general comment goes for the much of the rest of the code below; while you are adding a bunch of (valuable) checks here, the style and indentation makes it hard to follow. -- Chris Lalancette