michel.ponceau | 2 Oct 2006 17:27
Picon

Proposal for virtual devices described in XML


Proposal modified after comments from Daniel P. Berrange:

/* Create a virtual device attachment to backend */
int virDomainAttachDevice(virDomainPtr domain, char *xml);
/* Destroy a virtual device attachment to backend */
int virDomainDetachDevice(virDomainPtr domain, char *xml);
/* <at> domain: pointer to domain object
* <at> xml: pointer to XML description of one device
* Returns 0 in case of success, -1 in case of failure. */

The XML would be in same form as the part of domain description for a single device, either <disk.../disk> or <interface.../interface>.
In Detach function, the device is identified by its target name for "disk", and its MAC address for "interface". When the MAC address has not been assigned by Libvirt user at device creation, it must be retrieved from Xen using virDomainGetXMLDesc function.
<div>
<br>Proposal modified after comments from Daniel P. Berrange:<br><br>
/* Create a virtual device attachment to backend */<br>
int virDomainAttachDevice(virDomainPtr domain, char *xml);<br>
/* Destroy a virtual device attachment to backend */<br>
int virDomainDetachDevice(virDomainPtr domain, char *xml);<br>
/*  <at> domain: pointer to domain object<br>
*  <at> xml: pointer to XML description of one device<br>
* Returns 0 in case of success, -1 in case of failure. */<br><br>
The XML would be in same form as the part of domain description for a single device, either &lt;disk.../disk&gt; or &lt;interface.../interface&gt;.<br>
In Detach function, the device is identified by its target name for "disk", and its MAC address for "interface". When the MAC address has not been assigned by Libvirt user at device creation, it must be retrieved from Xen using virDomainGetXMLDesc function.</div>
Daniel P. Berrange | 2 Oct 2006 20:31
Picon
Favicon

Re: Problem with hypercall

On Thu, Sep 28, 2006 at 11:37:42PM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 28, 2006 at 05:22:08PM -0400, Daniel Veillard wrote:
> > On Thu, Sep 28, 2006 at 09:43:19PM +0100, Daniel P. Berrange wrote:
> > > This is really quite a nasty problem, because the struct is passed into from
> > > numerous locations in the xen_internal.h code & I didn't want to cover the
> > > entire source with conditionals.
> > > 
> > > So what I've done is declared a new xen_v2_domaininfo struct which is
> > > the same as xen_v0_domaininfo, but with Jim's patch reverted again.
> > > Then provide two unions
> > 
> >   Okay, I really expected they didn't broke that data structure too,
> > sigh ... Yeah go ahead that seems the less uglier possible !
> 
> Ok, I comitted this to CVS now.
> 
> > I wonder why I didn't catch that, maybe my 3.0.3 test was done on an x86_64,
> 
> Luck. On one of my 32-bit 3.0.3 boxes the 0.1.6 appeared to work fine for the
> basic operations - it was just giving bogus data (eg, 153 cpus). Depending on
> how many domains were running & what their data was things either silently
> worked (but bogus data), or completely crashed & burned.
> 
> Anyway, I've now tested this on both versions of HV, and run through valgrind
> too as an extra sanity check. Lets just hope Xen doesn't break ABI yet again
> in the future....

Updating for this ABI breakage is beginning to feel like a never ending
problem :-(  It seems I missed one change in my last patch - mlock()'ing
the wrong bit of data for getdomainlist. By (bad) luck it just happened
to work anyway on my tests because I guess there was sufficient allocated
memory that mlock() was still in range. Running unprivileged via the proxy
though, the proxy will always request info on 1020 domains (even if only
a couple are running), which meant we mlock() several 10's of KB of data.
THis ran in an area on unallocated mem & caused mlock() to fail, hence
my discovering the problem.

Anyway, attached is the patch I've applied to CVS...

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
Index: xen_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xen_internal.c,v
retrieving revision 1.43
diff -u -r1.43 xen_internal.c
--- xen_internal.c	29 Sep 2006 16:12:08 -0000	1.43
+++ xen_internal.c	2 Oct 2006 19:25:01 -0000
 <at>  <at>  -145,7 +145,15  <at>  <at> 
      domlist.v0[n].domain :                         \
      domlist.v2[n].domain)

+#define XEN_GETDOMAININFOLIST_DATA(domlist)     \
+    (hypervisor_version < 2 ?                   \
+     (void*)(domlist->v0) :                     \
+     (void*)(domlist->v2))

+#define XEN_GETDOMAININFO_SIZE                  \
+    (hypervisor_version < 2 ?                   \
+     sizeof(xen_v0_getdomaininfo) :             \
+     sizeof(xen_v2_getdomaininfo))

 #define XEN_GETDOMAININFO_CLEAR(dominfo)                        \
     (hypervisor_version < 2 ?                                   \
 <at>  <at>  -645,7 +653,8  <at>  <at> 
 {
     int ret = -1;

-    if (mlock(dominfos, sizeof(xen_v0_getdomaininfo) * maxids) < 0) {
+    if (mlock(XEN_GETDOMAININFOLIST_DATA(dominfos),
+              XEN_GETDOMAININFO_SIZE * maxids) < 0) {
         virXenError(VIR_ERR_XEN_CALL, " locking",
                     sizeof(xen_v0_getdomaininfo) * maxids);
         return (-1);
 <at>  <at>  -687,7 +696,8  <at>  <at> 
         if (ret == 0)
             ret = op.u.getdomaininfolist.num_domains;
     }
-    if (munlock(dominfos, sizeof(xen_v0_getdomaininfo) * maxids) < 0) {
+    if (mlock(XEN_GETDOMAININFOLIST_DATA(dominfos),
+              XEN_GETDOMAININFO_SIZE * maxids) < 0) {
         virXenError(VIR_ERR_XEN_CALL, " release",
                     sizeof(xen_v0_getdomaininfo));
         ret = -1;
Index: xen_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xen_internal.c,v
retrieving revision 1.43
diff -u -r1.43 xen_internal.c
--- xen_internal.c	29 Sep 2006 16:12:08 -0000	1.43
+++ xen_internal.c	2 Oct 2006 19:25:01 -0000
 <at>  <at>  -145,7 +145,15  <at>  <at> 
      domlist.v0[n].domain :                         \
      domlist.v2[n].domain)

+#define XEN_GETDOMAININFOLIST_DATA(domlist)     \
+    (hypervisor_version < 2 ?                   \
+     (void*)(domlist->v0) :                     \
+     (void*)(domlist->v2))

+#define XEN_GETDOMAININFO_SIZE                  \
+    (hypervisor_version < 2 ?                   \
+     sizeof(xen_v0_getdomaininfo) :             \
+     sizeof(xen_v2_getdomaininfo))

 #define XEN_GETDOMAININFO_CLEAR(dominfo)                        \
     (hypervisor_version < 2 ?                                   \
 <at>  <at>  -645,7 +653,8  <at>  <at> 
 {
     int ret = -1;

-    if (mlock(dominfos, sizeof(xen_v0_getdomaininfo) * maxids) < 0) {
+    if (mlock(XEN_GETDOMAININFOLIST_DATA(dominfos),
+              XEN_GETDOMAININFO_SIZE * maxids) < 0) {
         virXenError(VIR_ERR_XEN_CALL, " locking",
                     sizeof(xen_v0_getdomaininfo) * maxids);
         return (-1);
 <at>  <at>  -687,7 +696,8  <at>  <at> 
         if (ret == 0)
             ret = op.u.getdomaininfolist.num_domains;
     }
-    if (munlock(dominfos, sizeof(xen_v0_getdomaininfo) * maxids) < 0) {
+    if (mlock(XEN_GETDOMAININFOLIST_DATA(dominfos),
+              XEN_GETDOMAININFO_SIZE * maxids) < 0) {
         virXenError(VIR_ERR_XEN_CALL, " release",
                     sizeof(xen_v0_getdomaininfo));
         ret = -1;
Daniel Veillard | 2 Oct 2006 23:26
Picon
Favicon
Gravatar

Re: Problem with hypercall

On Mon, Oct 02, 2006 at 07:31:41PM +0100, Daniel P. Berrange wrote:
> Updating for this ABI breakage is beginning to feel like a never ending
> problem :-(  It seems I missed one change in my last patch - mlock()'ing
> the wrong bit of data for getdomainlist. By (bad) luck it just happened
> to work anyway on my tests because I guess there was sufficient allocated
> memory that mlock() was still in range. Running unprivileged via the proxy
> though, the proxy will always request info on 1020 domains (even if only
> a couple are running), which meant we mlock() several 10's of KB of data.
> THis ran in an area on unallocated mem & caused mlock() to fail, hence
> my discovering the problem.

  urgh !

> Anyway, attached is the patch I've applied to CVS...

  Thanks for spotting this! I fixed the munlock -> mlock transition :-)
and updated the error messages to report the new sizes,

Daniel

--

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard <at> redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

Sargun Dhillon | 4 Oct 2006 04:15
Picon

Xen Routed

Does the XML file format support routed?
If so, what would the <interface> clause look like?

--
Sargun Dhillon
President
Atarack Communications, Inc.
(925)-202-9485

<div><p>Does the XML file format support routed?<br>If so, what would the &lt;interface&gt; clause look like?<br clear="all"><br>-- <br>Sargun Dhillon<br>President<br>Atarack Communications, Inc.<br>(925)-202-9485
</p></div>
Daniel P. Berrange | 4 Oct 2006 15:06
Picon
Favicon

Re: Xen Routed

On Tue, Oct 03, 2006 at 07:15:23PM -0700, Sargun Dhillon wrote:
> Does the XML file format support routed?
> If so, what would the <interface> clause look like?

I'm not familiar with the way routed networking is setup with Xen. If you
can provide the example VIF device config params from /etc/xen/≤some guest>
which are needed for routing I'll check if they map through to libvirt
XML format in neccessary manner.

Regards,
Dan.
--

-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Daniel P. Berrange | 4 Oct 2006 21:16
Picon
Favicon

Re: Xen Routed

On Wed, Oct 04, 2006 at 08:09:27AM -0600, Nick Devito wrote:
> On Wed, 2006-10-04 at 14:06 +0100, Daniel P. Berrange wrote:
> > On Tue, Oct 03, 2006 at 07:15:23PM -0700, Sargun Dhillon wrote:
> > > Does the XML file format support routed?
> > > If so, what would the <interface> clause look like?
> > 
> > I'm not familiar with the way routed networking is setup with Xen. If you
> > can provide the example VIF device config params from /etc/xen/≤some guest>
> > which are needed for routing I'll check if they map through to libvirt
> > XML format in neccessary manner.
> > 
> Xen routed requires the ip= configuration variable, and, I think thats
> it. I don't see any reason why it shouldn't work correctly. 

Ok, looking at the XML parsing routines we don't currently support the
'ip' property.  Fortunately, it looks as if one of Michel Ponceau's
patches from a thread last week ought to solve this problem, so we ought
to be able to get this working for next release.

Regards,
Dan.
--

-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Daniel P. Berrange | 5 Oct 2006 20:23
Picon
Favicon

Re: Supporting new Xen 3.0.3 blktap backend for file devices

On Wed, Sep 27, 2006 at 06:36:38PM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 27, 2006 at 01:16:17PM -0400, Daniel Veillard wrote:
> > On Wed, Sep 27, 2006 at 07:05:38PM +0200, Karel Zak wrote:
> > > On Tue, Sep 26, 2006 at 08:08:33PM +0100, Daniel P. Berrange wrote:
> > > > 
> > > > This means the easy option of just making all file devices use blktap in
> > > > 3.0.3 is not practical. This in turns means we need to  come up with a
> > > > way to express the new driver methods in libvirt XML. There are a few
> > > > options I can think of :
> > > > 
> > > >  - Allow more values in the 'type' attribute, eg file, block, blktap:aio,
> > > >    blktap:qcow, etc. This feels wrong because the blktap:* entries are
> > > >    really still 'file' types.
> > > > 
> > > >  - Introduce a new attribute  'driver'  where you can specify 'loop',
> > > >    'blktap:aio', 'blktap:qcow', etc
> > > > 
> > > >  - Introduce a new element 'driver' where you can specify the same
> > > > 
> > > >        <disk type="file">
> > > >           <driver type='blktap:aio'/>
> > > >        </disk>
> > > > 
> > > >  - Same as above, but normalize the driver type further
> > > > 
> > > >        <disk type="file">
> > > >          <driver type='blktap' backend='aio' />
> > > >        </disk>
> > > > 
> > > > My preference is probably option 3 or 4.
> > > 
> > >  My preference is definitely 4.
> > >  
> > >  (well, I'm affected by Database Normalization where it's bad merge
> > >  two information into one attribute -- so 'blktap:aio' is wrong way
> > >  ;-)
> > 
> >   I would agree that rather than glueing it's better to separate the 2 strings
> > as separate attributes. But should the second be named 'backend' or something
> > more generic like 'subtype'. That's the only slight concern I have but it's
> > probably better to be explicit about and the suggested form is just fine.
> > Let's go for 4/ :-)
> 
> Ok, looks like we have total agreement. I'll knock up a patch implementing
> option #4 and post it for review.

Attached is a patch which implements option 4. If no <driver> block is
supplied, then we continue existing behavior of using file: for file backed
disks, and phy: for block backed disks. Any of the following are then valid:

  <driver name='file'/>                ->     file:
  <driver name='phy'/>                 ->     phy:
  <driver name='tap'/>                 ->     tap:aio:
  <driver name='tap' type='aio'/>      ->     tap:aio:
  <driver name='tap' type='qcow'/>     ->     tap:qcow:

The only supported names are 'file', 'phy' & 'tap'. If the name is 'tap'
then it is valid to use an additional 'type' attributte.  We don't do
any checking on cotents of 'type' attribute, it is just passed straight
through to xend, since the blktap driver has a wide variety of types
available. When fetching XML from libvirt  you are now guarenteed to
be given a <driver> block in each disk.

The patch was rather tedious, because not only did the blktap stuff change
the prefix used on file paths in the (uname) SEXPR block, but it actually
changed the entire enclosing block from (vbd ...) to (tap ...) for some
crazy reason.

I'm not attaching them here because it would bloat the patch, but I've written
a tonne more testfiles for the xml <-> sexpr conversions to validate the 
patch is operating correctly.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
Index: src/xend_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xend_internal.c,v
retrieving revision 1.66
diff -u -r1.66 xend_internal.c
--- src/xend_internal.c	29 Sep 2006 12:00:58 -0000	1.66
+++ src/xend_internal.c	5 Oct 2006 18:12:33 -0000
 <at>  <at>  -1499,7 +1499,7  <at>  <at> 
 	for (i = 0, j = 0;(i < 32) && (tmp[j] != 0);j++) {
 	    if (((tmp[j] >= '0') && (tmp[j] <= '9')) ||
 	        ((tmp[j] >= 'a') && (tmp[j] <= 'f')))
-		compact[i++] = tmp[j];
+            compact[i++] = tmp[j];
 	    else if ((tmp[j] >= 'A') && (tmp[j] <= 'F'))
 	        compact[i++] = tmp[j] + 'a' - 'A';
 	}
 <at>  <at>  -1509,7 +1509,7  <at>  <at> 
     }
     tmp = sexpr_node(root, "domain/bootloader");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <bootloader>%s</bootloader>\n", tmp);
+        virBufferVSprintf(&buf, "  <bootloader>%s</bootloader>\n", tmp);

     if (sexpr_lookup(root, "domain/image")) {
         hvm = sexpr_lookup(root, "domain/image/hvm") ? 1 : 0;
 <at>  <at>  -1522,13 +1522,13  <at>  <at> 
                       sexpr_int(root, "domain/vcpus"));
     tmp = sexpr_node(root, "domain/on_poweroff");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <on_poweroff>%s</on_poweroff>\n", tmp);
+        virBufferVSprintf(&buf, "  <on_poweroff>%s</on_poweroff>\n", tmp);
     tmp = sexpr_node(root, "domain/on_reboot");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <on_reboot>%s</on_reboot>\n", tmp);
+        virBufferVSprintf(&buf, "  <on_reboot>%s</on_reboot>\n", tmp);
     tmp = sexpr_node(root, "domain/on_crash");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <on_crash>%s</on_crash>\n", tmp);
+        virBufferVSprintf(&buf, "  <on_crash>%s</on_crash>\n", tmp);

     if (hvm) {
         virBufferAdd(&buf, "  <features>\n", 13);
 <at>  <at>  -1546,95 +1546,116  <at>  <at> 
     /* in case of HVM we have devices emulation */
     tmp = sexpr_node(root, "domain/image/hvm/device_model");
     if ((tmp != NULL) && (tmp[0] != 0))
-	virBufferVSprintf(&buf, "    <emulator>%s</emulator>\n", tmp);
+        virBufferVSprintf(&buf, "    <emulator>%s</emulator>\n", tmp);

     for (cur = root; cur->kind == SEXPR_CONS; cur = cur->cdr) {
         node = cur->car;
-        if (sexpr_lookup(node, "device/vbd")) {
-            tmp = sexpr_node(node, "device/vbd/uname");
-            if (tmp == NULL)
-                continue;
-            if (!memcmp(tmp, "file:", 5)) {
-                int cdrom = 0;
-                const char *src = tmp+5;
-                const char *dst = sexpr_node(node, "device/vbd/dev");
-
-                if (dst == NULL) {
-                    virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
-                                 _("domain information incomplete, vbd has no dev"));
-                    goto error;
-                }
+        if (sexpr_lookup(node, "device/vbd") ||
+            sexpr_lookup(node, "device/tap")) {
+            char *offset;
+            int isBlock = 0;
+            int cdrom = 0;
+            char *drvName = NULL;
+            char *drvType = NULL;
+            const char *src = NULL;
+            const char *dst = NULL;
+            const char *mode = NULL;
+            /* Why, oh why did this need to change as well as the
+               specifying tap in the (uname..) block ??!!?! Crazy
+               Xen formats :-( */
+            if (sexpr_lookup(node, "device/vbd")) {
+                src = sexpr_node(node, "device/vbd/uname");
+                dst = sexpr_node(node, "device/vbd/dev");
+                mode = sexpr_node(node, "device/vbd/mode");
+            } else {
+                src = sexpr_node(node, "device/tap/uname");
+                dst = sexpr_node(node, "device/tap/dev");
+                mode = sexpr_node(node, "device/tap/mode");
+            }

-                if (!strncmp(dst, "ioemu:", 6)) 
-                    dst += 6;
-                /* New style disk config from Xen >= 3.0.3 */
-                if (xendConfigVersion > 1) {
-                    char *offset = rindex(dst, ':');
-                    if (offset) {
-                        if (!strcmp(offset, ":cdrom")) {
-                            cdrom = 1;
-                        } else if (!strcmp(offset, ":disk")) {
-                            /* defualt anyway */
-                        } else {
-                            /* Unknown, lets pretend its a disk */
-                        }
-                        offset[0] = '\0';
-                    }
-                }
+            if (src == NULL) {
+                virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
+                             _("domain information incomplete, vbd has no src"));
+                goto bad_parse;
+            }

-                virBufferVSprintf(&buf, "    <disk type='file' device='%s'>\n", cdrom ? "cdrom" : "disk");
-                virBufferVSprintf(&buf, "      <source file='%s'/>\n", src);
-                virBufferVSprintf(&buf, "      <target dev='%s'/>\n", dst);
-                tmp = sexpr_node(node, "device/vbd/mode");
-                /* XXX should we force mode == r, if cdrom==1, or assume
-                   xend has already done this ? */
-                if ((tmp != NULL) && (!strcmp(tmp, "r")))
-                    virBufferVSprintf(&buf, "      <readonly/>\n");
-                virBufferAdd(&buf, "    </disk>\n", 12);
-            } else if (!memcmp(tmp, "phy:", 4)) {
-                int cdrom = 0;
-                const char *src = tmp+4;
-                const char *dst = sexpr_node(node, "device/vbd/dev");
-
-                if (dst == NULL) {
-                    virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
-                                 _("domain information incomplete, vbd has no dev"));
-                    goto error;
-                }
+            if (dst == NULL) {
+                virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
+                             _("domain information incomplete, vbd has no dev"));
+                goto bad_parse;
+            }
+
+
+            offset = strchr(src, ':');
+            if (!offset)
+                goto bad_parse;
+
+            drvName = malloc((offset-src)+1);
+            strncpy(drvName, src, (offset-src));
+            drvName[offset-src] = '\0';
+
+            src = offset + 1;
+
+            if (!strcmp(drvName, "tap")) {
+                offset = strchr(src, ':');
+                if (!offset)
+                    goto bad_parse;
+
+                drvType = malloc((offset-src)+1);
+                strncpy(drvType, src, (offset-src));
+                drvType[offset-src] = '\0';
+                src = offset + 1;
+            } else if (!strcmp(drvName, "phy")) {
+                isBlock = 1;
+            } else if (!strcmp(drvName, "file")) {
+                isBlock = 0;
+            }

-                if (!strncmp(dst, "ioemu:", 6)) 
-                    dst += 6;
-                /* New style cdrom config from Xen >= 3.0.3 */
-                if (xendConfigVersion > 1) {
-                    char *offset = rindex(dst, ':');
-                    if (offset) {
-                        if (!strcmp(offset, ":cdrom")) {
-                            cdrom = 1;
-                        } else if (!strcmp(offset, ":disk")) {
-                            /* defualt anyway */
-                        } else {
-                            /* Unknown, lets pretend its a disk */
-                        }
-                        offset[0] = '\0';
+            if (!strncmp(dst, "ioemu:", 6))
+                dst += 6;
+
+            /* New style disk config from Xen >= 3.0.3 */
+            if (xendConfigVersion > 1) {
+                offset = rindex(dst, ':');
+                if (offset) {
+                    if (!strcmp(offset, ":cdrom")) {
+                        cdrom = 1;
+                    } else if (!strcmp(offset, ":disk")) {
+                        /* defualt anyway */
+                    } else {
+                        /* Unknown, lets pretend its a disk */
                     }
+                    offset[0] = '\0';
                 }
+            }

-                virBufferVSprintf(&buf, "    <disk type='block' device='%s'>\n", cdrom ? "cdrom" : "disk");
+            virBufferVSprintf(&buf, "    <disk type='%s' device='%s'>\n",
+                              isBlock ? "block" : "file",
+                              cdrom ? "cdrom" : "disk");
+            if (drvType) {
+                virBufferVSprintf(&buf, "      <driver name='%s' type='%s'/>\n", drvName, drvType);
+            } else {
+                virBufferVSprintf(&buf, "      <driver name='%s'/>\n", drvName);
+            }
+            if (isBlock) {
                 virBufferVSprintf(&buf, "      <source dev='%s'/>\n", src);
-                virBufferVSprintf(&buf, "      <target dev='%s'/>\n", dst);
-                tmp = sexpr_node(node, "device/vbd/mode");
-                /* XXX should we force mode == r, if cdrom==1, or assume
-                   xend has already done this ? */
-                if ((tmp != NULL) && (!strcmp(tmp, "r")))
-                    virBufferVSprintf(&buf, "      <readonly/>\n");
-                virBufferAdd(&buf, "    </disk>\n", 12);
             } else {
-                char serial[1000];
+                virBufferVSprintf(&buf, "      <source file='%s'/>\n", src);
+            }
+            virBufferVSprintf(&buf, "      <target dev='%s'/>\n", dst);

-                TODO sexpr2string(node, serial, 1000);
-                virBufferVSprintf(&buf, "<!-- Failed to parse %s -->\n",
-                                  serial);
-            TODO}
+
+            /* XXX should we force mode == r, if cdrom==1, or assume
+               xend has already done this ? */
+            if ((mode != NULL) && (!strcmp(mode, "r")))
+                virBufferVSprintf(&buf, "      <readonly/>\n");
+            virBufferAdd(&buf, "    </disk>\n", 12);
+
+        bad_parse:
+            if (drvName)
+                free(drvName);
+            if (drvType)
+                free(drvType);
         } else if (sexpr_lookup(node, "device/vif")) {
 	    const char *tmp2;

 <at>  <at>  -1692,6 +1713,7  <at>  <at> 
             tmp = sexpr_node(root, "domain/image/hvm/cdrom");
             if ((tmp != NULL) && (tmp[0] != 0)) {
                 virBufferAdd(&buf, "    <disk type='file' device='cdrom'>\n", 38);
+                virBufferAdd(&buf, "      <driver name='file'/>\n", 28);
                 virBufferVSprintf(&buf, "      <source file='%s'/>\n", tmp);
                 virBufferAdd(&buf, "      <target dev='hdc'/>\n", 26);
                 virBufferAdd(&buf, "      <readonly/>\n", 18);
Index: src/xml.c
===================================================================
RCS file: /data/cvs/libvirt/src/xml.c,v
retrieving revision 1.41
diff -u -r1.41 xml.c
--- src/xml.c	21 Sep 2006 15:24:37 -0000	1.41
+++ src/xml.c	5 Oct 2006 18:12:34 -0000
 <at>  <at>  -921,6 +921,8  <at>  <at> 
     xmlChar *device = NULL;
     xmlChar *source = NULL;
     xmlChar *target = NULL;
+    xmlChar *drvName = NULL;
+    xmlChar *drvType = NULL;
     int ro = 0;
     int typ = 0;
     int cdrom = 0;
 <at>  <at>  -948,6 +950,11  <at>  <at> 
             } else if ((target == NULL) &&
                        (xmlStrEqual(cur->name, BAD_CAST "target"))) {
                 target = xmlGetProp(cur, BAD_CAST "dev");
+            } else if ((drvName == NULL) &&
+                       (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
+                drvName = xmlGetProp(cur, BAD_CAST "name");
+                if (!strcmp((const char *)drvName, "tap"))
+                    drvType = xmlGetProp(cur, BAD_CAST "type");
             } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
                 ro = 1;
             }
 <at>  <at>  -972,14 +979,14  <at>  <at> 
     /* Xend (all versions) put the floppy device config
      * under the hvm (image (os)) block
      */
-    if (hvm && 
+    if (hvm &&
         device &&
         !strcmp((const char *)device, "floppy")) {
         return 0;
     }

     /* Xend <= 3.0.2 doesn't include cdrom config here */
-    if (hvm && 
+    if (hvm &&
         device &&
         !strcmp((const char *)device, "cdrom")) {
         if (xendConfigVersion == 1)
 <at>  <at>  -990,7 +997,14  <at>  <at> 

 
     virBufferAdd(buf, "(device ", 8);
-    virBufferAdd(buf, "(vbd ", 5);
+    /* Why, oh why did this need to change as well as the
+       specifying tap in the (uname..) block ??!!?! Crazy
+       Xen formats :-( */
+    if (drvName && !strcmp((const char *)drvName, "tap")) {
+        virBufferAdd(buf, "(tap ", 5);
+    } else {
+        virBufferAdd(buf, "(vbd ", 5);
+    }

     if (hvm) {
         char *tmp = (char *)target;
 <at>  <at>  -1000,19 +1014,32  <at>  <at> 

         /* Xend <= 3.0.2 wants a ioemu: prefix on devices for HVM */
         if (xendConfigVersion == 1)
-            virBufferVSprintf(buf, "(dev 'ioemu:%s')", (const char *) tmp);
+            virBufferVSprintf(buf, "(dev 'ioemu:%s')", (const char *)tmp);
         else /* But newer does not */
-            virBufferVSprintf(buf, "(dev '%s%s')", (const char *) tmp, cdrom ? ":cdrom" : ":disk");
+            virBufferVSprintf(buf, "(dev '%s%s')", (const char *)tmp, cdrom ? ":cdrom" : ":disk");
     } else
-        virBufferVSprintf(buf, "(dev '%s')", (const char *) target);
+        virBufferVSprintf(buf, "(dev '%s')", (const char *)target);

-    if (typ == 0)
-        virBufferVSprintf(buf, "(uname 'file:%s')", source);
-    else if (typ == 1) {
-        if (source[0] == '/')
-            virBufferVSprintf(buf, "(uname 'phy:%s')", source);
-        else
-            virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source);
+    if (drvName) {
+        if (!strcmp((const char *)drvName, "tap")) {
+            virBufferVSprintf(buf, "(uname '%s:%s:%s')",
+                              (const char *)drvName,
+                              (drvType ? (const char *)drvType : "aio"),
+                              (const char *)source);
+        } else {
+            virBufferVSprintf(buf, "(uname '%s:%s')",
+                              (const char *)drvName,
+                              (const char *)source);
+        }
+    } else {
+        if (typ == 0)
+            virBufferVSprintf(buf, "(uname 'file:%s')", source);
+        else if (typ == 1) {
+            if (source[0] == '/')
+                virBufferVSprintf(buf, "(uname 'phy:%s')", source);
+            else
+                virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source);
+        }
     }
     if (ro == 0)
         virBufferVSprintf(buf, "(mode 'w')");
 <at>  <at>  -1021,6 +1048,9  <at>  <at> 

     virBufferAdd(buf, ")", 1);
     virBufferAdd(buf, ")", 1);
+    xmlFree(drvType);
+    xmlFree(drvName);
+    xmlFree(device);
     xmlFree(target);
     xmlFree(source);
     return (0);
Index: src/xend_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xend_internal.c,v
retrieving revision 1.66
diff -u -r1.66 xend_internal.c
--- src/xend_internal.c	29 Sep 2006 12:00:58 -0000	1.66
+++ src/xend_internal.c	5 Oct 2006 18:12:33 -0000
 <at>  <at>  -1499,7 +1499,7  <at>  <at> 
 	for (i = 0, j = 0;(i < 32) && (tmp[j] != 0);j++) {
 	    if (((tmp[j] >= '0') && (tmp[j] <= '9')) ||
 	        ((tmp[j] >= 'a') && (tmp[j] <= 'f')))
-		compact[i++] = tmp[j];
+            compact[i++] = tmp[j];
 	    else if ((tmp[j] >= 'A') && (tmp[j] <= 'F'))
 	        compact[i++] = tmp[j] + 'a' - 'A';
 	}
 <at>  <at>  -1509,7 +1509,7  <at>  <at> 
     }
     tmp = sexpr_node(root, "domain/bootloader");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <bootloader>%s</bootloader>\n", tmp);
+        virBufferVSprintf(&buf, "  <bootloader>%s</bootloader>\n", tmp);

     if (sexpr_lookup(root, "domain/image")) {
         hvm = sexpr_lookup(root, "domain/image/hvm") ? 1 : 0;
 <at>  <at>  -1522,13 +1522,13  <at>  <at> 
                       sexpr_int(root, "domain/vcpus"));
     tmp = sexpr_node(root, "domain/on_poweroff");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <on_poweroff>%s</on_poweroff>\n", tmp);
+        virBufferVSprintf(&buf, "  <on_poweroff>%s</on_poweroff>\n", tmp);
     tmp = sexpr_node(root, "domain/on_reboot");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <on_reboot>%s</on_reboot>\n", tmp);
+        virBufferVSprintf(&buf, "  <on_reboot>%s</on_reboot>\n", tmp);
     tmp = sexpr_node(root, "domain/on_crash");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <on_crash>%s</on_crash>\n", tmp);
+        virBufferVSprintf(&buf, "  <on_crash>%s</on_crash>\n", tmp);

     if (hvm) {
         virBufferAdd(&buf, "  <features>\n", 13);
 <at>  <at>  -1546,95 +1546,116  <at>  <at> 
     /* in case of HVM we have devices emulation */
     tmp = sexpr_node(root, "domain/image/hvm/device_model");
     if ((tmp != NULL) && (tmp[0] != 0))
-	virBufferVSprintf(&buf, "    <emulator>%s</emulator>\n", tmp);
+        virBufferVSprintf(&buf, "    <emulator>%s</emulator>\n", tmp);

     for (cur = root; cur->kind == SEXPR_CONS; cur = cur->cdr) {
         node = cur->car;
-        if (sexpr_lookup(node, "device/vbd")) {
-            tmp = sexpr_node(node, "device/vbd/uname");
-            if (tmp == NULL)
-                continue;
-            if (!memcmp(tmp, "file:", 5)) {
-                int cdrom = 0;
-                const char *src = tmp+5;
-                const char *dst = sexpr_node(node, "device/vbd/dev");
-
-                if (dst == NULL) {
-                    virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
-                                 _("domain information incomplete, vbd has no dev"));
-                    goto error;
-                }
+        if (sexpr_lookup(node, "device/vbd") ||
+            sexpr_lookup(node, "device/tap")) {
+            char *offset;
+            int isBlock = 0;
+            int cdrom = 0;
+            char *drvName = NULL;
+            char *drvType = NULL;
+            const char *src = NULL;
+            const char *dst = NULL;
+            const char *mode = NULL;
+            /* Why, oh why did this need to change as well as the
+               specifying tap in the (uname..) block ??!!?! Crazy
+               Xen formats :-( */
+            if (sexpr_lookup(node, "device/vbd")) {
+                src = sexpr_node(node, "device/vbd/uname");
+                dst = sexpr_node(node, "device/vbd/dev");
+                mode = sexpr_node(node, "device/vbd/mode");
+            } else {
+                src = sexpr_node(node, "device/tap/uname");
+                dst = sexpr_node(node, "device/tap/dev");
+                mode = sexpr_node(node, "device/tap/mode");
+            }

-                if (!strncmp(dst, "ioemu:", 6)) 
-                    dst += 6;
-                /* New style disk config from Xen >= 3.0.3 */
-                if (xendConfigVersion > 1) {
-                    char *offset = rindex(dst, ':');
-                    if (offset) {
-                        if (!strcmp(offset, ":cdrom")) {
-                            cdrom = 1;
-                        } else if (!strcmp(offset, ":disk")) {
-                            /* defualt anyway */
-                        } else {
-                            /* Unknown, lets pretend its a disk */
-                        }
-                        offset[0] = '\0';
-                    }
-                }
+            if (src == NULL) {
+                virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
+                             _("domain information incomplete, vbd has no src"));
+                goto bad_parse;
+            }

-                virBufferVSprintf(&buf, "    <disk type='file' device='%s'>\n", cdrom ? "cdrom" : "disk");
-                virBufferVSprintf(&buf, "      <source file='%s'/>\n", src);
-                virBufferVSprintf(&buf, "      <target dev='%s'/>\n", dst);
-                tmp = sexpr_node(node, "device/vbd/mode");
-                /* XXX should we force mode == r, if cdrom==1, or assume
-                   xend has already done this ? */
-                if ((tmp != NULL) && (!strcmp(tmp, "r")))
-                    virBufferVSprintf(&buf, "      <readonly/>\n");
-                virBufferAdd(&buf, "    </disk>\n", 12);
-            } else if (!memcmp(tmp, "phy:", 4)) {
-                int cdrom = 0;
-                const char *src = tmp+4;
-                const char *dst = sexpr_node(node, "device/vbd/dev");
-
-                if (dst == NULL) {
-                    virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
-                                 _("domain information incomplete, vbd has no dev"));
-                    goto error;
-                }
+            if (dst == NULL) {
+                virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
+                             _("domain information incomplete, vbd has no dev"));
+                goto bad_parse;
+            }
+
+
+            offset = strchr(src, ':');
+            if (!offset)
+                goto bad_parse;
+
+            drvName = malloc((offset-src)+1);
+            strncpy(drvName, src, (offset-src));
+            drvName[offset-src] = '\0';
+
+            src = offset + 1;
+
+            if (!strcmp(drvName, "tap")) {
+                offset = strchr(src, ':');
+                if (!offset)
+                    goto bad_parse;
+
+                drvType = malloc((offset-src)+1);
+                strncpy(drvType, src, (offset-src));
+                drvType[offset-src] = '\0';
+                src = offset + 1;
+            } else if (!strcmp(drvName, "phy")) {
+                isBlock = 1;
+            } else if (!strcmp(drvName, "file")) {
+                isBlock = 0;
+            }

-                if (!strncmp(dst, "ioemu:", 6)) 
-                    dst += 6;
-                /* New style cdrom config from Xen >= 3.0.3 */
-                if (xendConfigVersion > 1) {
-                    char *offset = rindex(dst, ':');
-                    if (offset) {
-                        if (!strcmp(offset, ":cdrom")) {
-                            cdrom = 1;
-                        } else if (!strcmp(offset, ":disk")) {
-                            /* defualt anyway */
-                        } else {
-                            /* Unknown, lets pretend its a disk */
-                        }
-                        offset[0] = '\0';
+            if (!strncmp(dst, "ioemu:", 6))
+                dst += 6;
+
+            /* New style disk config from Xen >= 3.0.3 */
+            if (xendConfigVersion > 1) {
+                offset = rindex(dst, ':');
+                if (offset) {
+                    if (!strcmp(offset, ":cdrom")) {
+                        cdrom = 1;
+                    } else if (!strcmp(offset, ":disk")) {
+                        /* defualt anyway */
+                    } else {
+                        /* Unknown, lets pretend its a disk */
                     }
+                    offset[0] = '\0';
                 }
+            }

-                virBufferVSprintf(&buf, "    <disk type='block' device='%s'>\n", cdrom ? "cdrom" : "disk");
+            virBufferVSprintf(&buf, "    <disk type='%s' device='%s'>\n",
+                              isBlock ? "block" : "file",
+                              cdrom ? "cdrom" : "disk");
+            if (drvType) {
+                virBufferVSprintf(&buf, "      <driver name='%s' type='%s'/>\n", drvName, drvType);
+            } else {
+                virBufferVSprintf(&buf, "      <driver name='%s'/>\n", drvName);
+            }
+            if (isBlock) {
                 virBufferVSprintf(&buf, "      <source dev='%s'/>\n", src);
-                virBufferVSprintf(&buf, "      <target dev='%s'/>\n", dst);
-                tmp = sexpr_node(node, "device/vbd/mode");
-                /* XXX should we force mode == r, if cdrom==1, or assume
-                   xend has already done this ? */
-                if ((tmp != NULL) && (!strcmp(tmp, "r")))
-                    virBufferVSprintf(&buf, "      <readonly/>\n");
-                virBufferAdd(&buf, "    </disk>\n", 12);
             } else {
-                char serial[1000];
+                virBufferVSprintf(&buf, "      <source file='%s'/>\n", src);
+            }
+            virBufferVSprintf(&buf, "      <target dev='%s'/>\n", dst);

-                TODO sexpr2string(node, serial, 1000);
-                virBufferVSprintf(&buf, "<!-- Failed to parse %s -->\n",
-                                  serial);
-            TODO}
+
+            /* XXX should we force mode == r, if cdrom==1, or assume
+               xend has already done this ? */
+            if ((mode != NULL) && (!strcmp(mode, "r")))
+                virBufferVSprintf(&buf, "      <readonly/>\n");
+            virBufferAdd(&buf, "    </disk>\n", 12);
+
+        bad_parse:
+            if (drvName)
+                free(drvName);
+            if (drvType)
+                free(drvType);
         } else if (sexpr_lookup(node, "device/vif")) {
 	    const char *tmp2;

 <at>  <at>  -1692,6 +1713,7  <at>  <at> 
             tmp = sexpr_node(root, "domain/image/hvm/cdrom");
             if ((tmp != NULL) && (tmp[0] != 0)) {
                 virBufferAdd(&buf, "    <disk type='file' device='cdrom'>\n", 38);
+                virBufferAdd(&buf, "      <driver name='file'/>\n", 28);
                 virBufferVSprintf(&buf, "      <source file='%s'/>\n", tmp);
                 virBufferAdd(&buf, "      <target dev='hdc'/>\n", 26);
                 virBufferAdd(&buf, "      <readonly/>\n", 18);
Index: src/xml.c
===================================================================
RCS file: /data/cvs/libvirt/src/xml.c,v
retrieving revision 1.41
diff -u -r1.41 xml.c
--- src/xml.c	21 Sep 2006 15:24:37 -0000	1.41
+++ src/xml.c	5 Oct 2006 18:12:34 -0000
 <at>  <at>  -921,6 +921,8  <at>  <at> 
     xmlChar *device = NULL;
     xmlChar *source = NULL;
     xmlChar *target = NULL;
+    xmlChar *drvName = NULL;
+    xmlChar *drvType = NULL;
     int ro = 0;
     int typ = 0;
     int cdrom = 0;
 <at>  <at>  -948,6 +950,11  <at>  <at> 
             } else if ((target == NULL) &&
                        (xmlStrEqual(cur->name, BAD_CAST "target"))) {
                 target = xmlGetProp(cur, BAD_CAST "dev");
+            } else if ((drvName == NULL) &&
+                       (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
+                drvName = xmlGetProp(cur, BAD_CAST "name");
+                if (!strcmp((const char *)drvName, "tap"))
+                    drvType = xmlGetProp(cur, BAD_CAST "type");
             } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
                 ro = 1;
             }
 <at>  <at>  -972,14 +979,14  <at>  <at> 
     /* Xend (all versions) put the floppy device config
      * under the hvm (image (os)) block
      */
-    if (hvm && 
+    if (hvm &&
         device &&
         !strcmp((const char *)device, "floppy")) {
         return 0;
     }

     /* Xend <= 3.0.2 doesn't include cdrom config here */
-    if (hvm && 
+    if (hvm &&
         device &&
         !strcmp((const char *)device, "cdrom")) {
         if (xendConfigVersion == 1)
 <at>  <at>  -990,7 +997,14  <at>  <at> 

 
     virBufferAdd(buf, "(device ", 8);
-    virBufferAdd(buf, "(vbd ", 5);
+    /* Why, oh why did this need to change as well as the
+       specifying tap in the (uname..) block ??!!?! Crazy
+       Xen formats :-( */
+    if (drvName && !strcmp((const char *)drvName, "tap")) {
+        virBufferAdd(buf, "(tap ", 5);
+    } else {
+        virBufferAdd(buf, "(vbd ", 5);
+    }

     if (hvm) {
         char *tmp = (char *)target;
 <at>  <at>  -1000,19 +1014,32  <at>  <at> 

         /* Xend <= 3.0.2 wants a ioemu: prefix on devices for HVM */
         if (xendConfigVersion == 1)
-            virBufferVSprintf(buf, "(dev 'ioemu:%s')", (const char *) tmp);
+            virBufferVSprintf(buf, "(dev 'ioemu:%s')", (const char *)tmp);
         else /* But newer does not */
-            virBufferVSprintf(buf, "(dev '%s%s')", (const char *) tmp, cdrom ? ":cdrom" : ":disk");
+            virBufferVSprintf(buf, "(dev '%s%s')", (const char *)tmp, cdrom ? ":cdrom" : ":disk");
     } else
-        virBufferVSprintf(buf, "(dev '%s')", (const char *) target);
+        virBufferVSprintf(buf, "(dev '%s')", (const char *)target);

-    if (typ == 0)
-        virBufferVSprintf(buf, "(uname 'file:%s')", source);
-    else if (typ == 1) {
-        if (source[0] == '/')
-            virBufferVSprintf(buf, "(uname 'phy:%s')", source);
-        else
-            virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source);
+    if (drvName) {
+        if (!strcmp((const char *)drvName, "tap")) {
+            virBufferVSprintf(buf, "(uname '%s:%s:%s')",
+                              (const char *)drvName,
+                              (drvType ? (const char *)drvType : "aio"),
+                              (const char *)source);
+        } else {
+            virBufferVSprintf(buf, "(uname '%s:%s')",
+                              (const char *)drvName,
+                              (const char *)source);
+        }
+    } else {
+        if (typ == 0)
+            virBufferVSprintf(buf, "(uname 'file:%s')", source);
+        else if (typ == 1) {
+            if (source[0] == '/')
+                virBufferVSprintf(buf, "(uname 'phy:%s')", source);
+            else
+                virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source);
+        }
     }
     if (ro == 0)
         virBufferVSprintf(buf, "(mode 'w')");
 <at>  <at>  -1021,6 +1048,9  <at>  <at> 

     virBufferAdd(buf, ")", 1);
     virBufferAdd(buf, ")", 1);
+    xmlFree(drvType);
+    xmlFree(drvName);
+    xmlFree(device);
     xmlFree(target);
     xmlFree(source);
     return (0);
Daniel P. Berrange | 6 Oct 2006 02:10
Picon
Favicon

Make python binding release GIL when using threads

In the virt-manager app I'm using the python bindings to libvirt for all
operations.  For long running operations such as creating new domains, or
saving/restoring domains I fork a background thread to do the call, so that
the main UI / status refresh can carry on working. Except this wasn't working.
The main thread was being blocked for the entire duration of the background
thread.  After a lot of nasty debug sessions I discovered that the main 
thread was being blocked in the GTK event loop attempting to aquire the
Python GIL (Global Interpreter Lock). The only way this could be occurring
is if the background thread doing the libvirt call held the GIL.

And indeed after looking at the source for the libvirt python bindings it
appears we never release the GIL when calling into the libvirt C apis, so
for the entire duration of virDomainCreateLinux  the python interpreter
is completely stopped. Clearly this sucks because virDomainCreateLinux
can take a very long time.

I read up a little on python threading & looked at how PyGTK deals with
this & have come up with a patch to make libvirt release the GIL when
entering a C call & re-aquire it on completion. Indeed, since PyGTK &
GTK are both LGPL, I just copied their C macros straight over. 

The patch appears to work - the python interpreter is no longer deadlocked
when calling virDomainCreateLinux (or any other libvirt calls). Unfortuntely
the virtmanager app still deadlocks, because it turns out XenD itself 
serializes all SEXPR HTTP requests, but once this is fixed the whole thing
is fully parallelized & works without blocking the app.

Attached the patch - I've not done much python threading before so let
me know if i missed anything / messed stuff up.  NB, I also hacked up
the error handling callback so it re-acquires the lock before calling
back into the python.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
? ABOUT-NLS
? config.rpath
? m4
? mkinstalldirs
? po/Makefile
? po/Makefile.in
? po/Makefile.in.in
? po/Makevars.template
? po/POTFILES
? po/Rules-quot
? po/boldquot.sed
? po/en <at> boldquot.header
? po/en <at> quot.header
? po/en_GB.gmo
? po/insert-header.sin
? po/quot.sed
? po/remove-potcdate.sin
? po/stamp-po
? tests/reconnect
Index: python/generator.py
===================================================================
RCS file: /data/cvs/libvirt/python/generator.py,v
retrieving revision 1.10
diff -r1.10 generator.py
424c424,425
<                                                               
---
> 
>     output.write("libvirt_begin_allow_threads;\n");
425a427
>     output.write("libvirt_end_allow_threads;\n");
Index: python/libvir.c
===================================================================
RCS file: /data/cvs/libvirt/python/libvir.c,v
retrieving revision 1.13
diff -r1.13 libvir.c
20a21,22
> 
> 
42a45,46
>     libvirt_ensure_thread_state;
> 
65a70,71
> 
>     libvirt_release_thread_state;
126a133
>     libvirt_begin_allow_threads;
127a135
>     libvirt_end_allow_threads;
142a151
>     libvirt_begin_allow_threads;
143a153
>     libvirt_end_allow_threads;
160a171
>     libvirt_begin_allow_threads;
161a173
>     libvirt_end_allow_threads;
184a197
>     libvirt_begin_allow_threads;
185a199
>     libvirt_end_allow_threads;
211a226
>     libvirt_begin_allow_threads;
212a228
>     libvirt_end_allow_threads;
234a251
>     int c_retval;
244c261,265
<     if (virDomainGetUUID(domain, &uuid[0]) < 0) {
---
>     libvirt_begin_allow_threads;
>     c_retval = virDomainGetUUID(domain, &uuid[0]);
>     libvirt_end_allow_threads;
> 
>     if (c_retval < 0) {
270a292
>     libvirt_begin_allow_threads;
271a294
>     libvirt_end_allow_threads;
Index: python/libvirt_wrap.h
===================================================================
RCS file: /data/cvs/libvirt/python/libvirt_wrap.h,v
retrieving revision 1.3
diff -r1.3 libvirt_wrap.h
50a51,102
> extern int libvirt_threads_enabled;
> 
> 
> /* Provide simple macro statement wrappers (adapted from Perl):
>  *  LIBVIRT_STMT_START { statements; } LIBVIRT_STMT_END;
>  *  can be used as a single statement, as in
>  *  if (x) LIBVIRT_STMT_START { ... } LIBVIRT_STMT_END; else ...
>  *
>  *  When GCC is compiling C code in non-ANSI mode, it will use the
>  *  compiler __extension__ to wrap the statements wihin `({' and '})' braces.
>  *  When compiling on platforms where configure has defined
>  *  HAVE_DOWHILE_MACROS, statements will be wrapped with `do' and `while (0)'.
>  *  For any other platforms (SunOS4 is known to have this issue), wrap the
>  *  statements with `if (1)' and `else (void) 0'.
>  */
> #if !(defined (LIBVIRT_STMT_START) && defined (LIBVIRT_STMT_END))
> # if defined (__GNUC__) && !defined (__STRICT_ANSI__) && !defined (__cplusplus)
> #  define LIBVIRT_STMT_START (void) __extension__ (
> #  define LIBVIRT_STMT_END )
> # else /* !(__GNUC__ && !__STRICT_ANSI__ && !__cplusplus) */
> #  if defined (HAVE_DOWHILE_MACROS)
> #   define LIBVIRT_STMT_START do
> #   define LIBVIRT_STMT_END while (0)
> #  else /* !HAVE_DOWHILE_MACROS */
> #   define LIBVIRT_STMT_START if (1)
> #   define LIBVIRT_STMT_END else (void) 0
> #  endif /* !HAVE_DOWHILE_MACROS */
> # endif /* !(__GNUC__ && !__STRICT_ANSI__ && !__cplusplus) */
> #endif
> 
> #define libvirt_begin_allow_threads			\
>   LIBVIRT_STMT_START {					\
>     PyThreadState *_save = NULL;			\
>     if (PyEval_ThreadsInitialized())			\
>       _save = PyEval_SaveThread();
> 
> #define libvirt_end_allow_threads                           \
>   if (PyEval_ThreadsInitialized())			    \
>     PyEval_RestoreThread(_save);			    \
>     } LIBVIRT_STMT_END
> 
> #define libvirt_ensure_thread_state			\
>   LIBVIRT_STMT_START {					\
>   PyGILState_STATE _save;				\
>     if (PyEval_ThreadsInitialized())			\
>       _save = PyGILState_Ensure();
> 
> #define libvirt_release_thread_state                           \
>   if (PyEval_ThreadsInitialized())			       \
>     PyGILState_Release(_save);				       \
>   } LIBVIRT_STMT_END
> 
? ABOUT-NLS
? config.rpath
? m4
? mkinstalldirs
? po/Makefile
? po/Makefile.in
? po/Makefile.in.in
? po/Makevars.template
? po/POTFILES
? po/Rules-quot
? po/boldquot.sed
? po/en <at> boldquot.header
? po/en <at> quot.header
? po/en_GB.gmo
? po/insert-header.sin
? po/quot.sed
? po/remove-potcdate.sin
? po/stamp-po
? tests/reconnect
Index: python/generator.py
===================================================================
RCS file: /data/cvs/libvirt/python/generator.py,v
retrieving revision 1.10
diff -r1.10 generator.py
424c424,425
<                                                               
---
> 
>     output.write("libvirt_begin_allow_threads;\n");
425a427
>     output.write("libvirt_end_allow_threads;\n");
Index: python/libvir.c
===================================================================
RCS file: /data/cvs/libvirt/python/libvir.c,v
retrieving revision 1.13
diff -r1.13 libvir.c
20a21,22
> 
> 
42a45,46
>     libvirt_ensure_thread_state;
> 
65a70,71
> 
>     libvirt_release_thread_state;
126a133
>     libvirt_begin_allow_threads;
127a135
>     libvirt_end_allow_threads;
142a151
>     libvirt_begin_allow_threads;
143a153
>     libvirt_end_allow_threads;
160a171
>     libvirt_begin_allow_threads;
161a173
>     libvirt_end_allow_threads;
184a197
>     libvirt_begin_allow_threads;
185a199
>     libvirt_end_allow_threads;
211a226
>     libvirt_begin_allow_threads;
212a228
>     libvirt_end_allow_threads;
234a251
>     int c_retval;
244c261,265
<     if (virDomainGetUUID(domain, &uuid[0]) < 0) {
---
>     libvirt_begin_allow_threads;
>     c_retval = virDomainGetUUID(domain, &uuid[0]);
>     libvirt_end_allow_threads;
> 
>     if (c_retval < 0) {
270a292
>     libvirt_begin_allow_threads;
271a294
>     libvirt_end_allow_threads;
Index: python/libvirt_wrap.h
===================================================================
RCS file: /data/cvs/libvirt/python/libvirt_wrap.h,v
retrieving revision 1.3
diff -r1.3 libvirt_wrap.h
50a51,102
> extern int libvirt_threads_enabled;
> 
> 
> /* Provide simple macro statement wrappers (adapted from Perl):
>  *  LIBVIRT_STMT_START { statements; } LIBVIRT_STMT_END;
>  *  can be used as a single statement, as in
>  *  if (x) LIBVIRT_STMT_START { ... } LIBVIRT_STMT_END; else ...
>  *
>  *  When GCC is compiling C code in non-ANSI mode, it will use the
>  *  compiler __extension__ to wrap the statements wihin `({' and '})' braces.
>  *  When compiling on platforms where configure has defined
>  *  HAVE_DOWHILE_MACROS, statements will be wrapped with `do' and `while (0)'.
>  *  For any other platforms (SunOS4 is known to have this issue), wrap the
>  *  statements with `if (1)' and `else (void) 0'.
>  */
> #if !(defined (LIBVIRT_STMT_START) && defined (LIBVIRT_STMT_END))
> # if defined (__GNUC__) && !defined (__STRICT_ANSI__) && !defined (__cplusplus)
> #  define LIBVIRT_STMT_START (void) __extension__ (
> #  define LIBVIRT_STMT_END )
> # else /* !(__GNUC__ && !__STRICT_ANSI__ && !__cplusplus) */
> #  if defined (HAVE_DOWHILE_MACROS)
> #   define LIBVIRT_STMT_START do
> #   define LIBVIRT_STMT_END while (0)
> #  else /* !HAVE_DOWHILE_MACROS */
> #   define LIBVIRT_STMT_START if (1)
> #   define LIBVIRT_STMT_END else (void) 0
> #  endif /* !HAVE_DOWHILE_MACROS */
> # endif /* !(__GNUC__ && !__STRICT_ANSI__ && !__cplusplus) */
> #endif
> 
> #define libvirt_begin_allow_threads			\
>   LIBVIRT_STMT_START {					\
>     PyThreadState *_save = NULL;			\
>     if (PyEval_ThreadsInitialized())			\
>       _save = PyEval_SaveThread();
> 
> #define libvirt_end_allow_threads                           \
>   if (PyEval_ThreadsInitialized())			    \
>     PyEval_RestoreThread(_save);			    \
>     } LIBVIRT_STMT_END
> 
> #define libvirt_ensure_thread_state			\
>   LIBVIRT_STMT_START {					\
>   PyGILState_STATE _save;				\
>     if (PyEval_ThreadsInitialized())			\
>       _save = PyGILState_Ensure();
> 
> #define libvirt_release_thread_state                           \
>   if (PyEval_ThreadsInitialized())			       \
>     PyGILState_Release(_save);				       \
>   } LIBVIRT_STMT_END
> 
Daniel Veillard | 6 Oct 2006 10:44
Picon
Favicon
Gravatar

Re: Supporting new Xen 3.0.3 blktap backend for file devices

On Thu, Oct 05, 2006 at 07:23:15PM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 27, 2006 at 06:36:38PM +0100, Daniel P. Berrange wrote:
> > Ok, looks like we have total agreement. I'll knock up a patch implementing
> > option #4 and post it for review.
> 
> Attached is a patch which implements option 4. If no <driver> block is
> supplied, then we continue existing behavior of using file: for file backed
> disks, and phy: for block backed disks. Any of the following are then valid:
> 
>   <driver name='file'/>                ->     file:
>   <driver name='phy'/>                 ->     phy:
>   <driver name='tap'/>                 ->     tap:aio:
>   <driver name='tap' type='aio'/>      ->     tap:aio:
>   <driver name='tap' type='qcow'/>     ->     tap:qcow:
> 
> The only supported names are 'file', 'phy' & 'tap'. If the name is 'tap'
> then it is valid to use an additional 'type' attributte.  We don't do
> any checking on cotents of 'type' attribute, it is just passed straight
> through to xend, since the blktap driver has a wide variety of types
> available. When fetching XML from libvirt  you are now guarenteed to
> be given a <driver> block in each disk.

  Okay,

> The patch was rather tedious, because not only did the blktap stuff change
> the prefix used on file paths in the (uname) SEXPR block, but it actually
> changed the entire enclosing block from (vbd ...) to (tap ...) for some
> crazy reason.

  Oh well ... at some point we will need to look at the new interfaces for
xend too, but one thing at a time :-)

> I'm not attaching them here because it would bloat the patch, but I've written
> a tonne more testfiles for the xml <-> sexpr conversions to validate the 
> patch is operating correctly.

  Great !

> Index: src/xend_internal.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/xend_internal.c,v
> retrieving revision 1.66
> diff -u -r1.66 xend_internal.c
> --- src/xend_internal.c	29 Sep 2006 12:00:58 -0000	1.66
> +++ src/xend_internal.c	5 Oct 2006 18:12:33 -0000
>  <at>  <at>  -1499,7 +1499,7  <at>  <at> 
>  	for (i = 0, j = 0;(i < 32) && (tmp[j] != 0);j++) {
>  	    if (((tmp[j] >= '0') && (tmp[j] <= '9')) ||
>  	        ((tmp[j] >= 'a') && (tmp[j] <= 'f')))
> -		compact[i++] = tmp[j];
> +            compact[i++] = tmp[j];

  maybe we should just add the full set of { } for the innner constructs 
too if reformatting.

>  	    else if ((tmp[j] >= 'A') && (tmp[j] <= 'F'))
>  	        compact[i++] = tmp[j] + 'a' - 'A';
>  	}
>  <at>  <at>  -1546,95 +1546,116  <at>  <at> 
> +            drvName = malloc((offset-src)+1);

  I agree that if we OOM there it's gonna be messy anyway but let's catch
NULL returns on allocs as much as possible

> +            strncpy(drvName, src, (offset-src));
> +            drvName[offset-src] = '\0';
> +
> +            src = offset + 1;
> +
> +            if (!strcmp(drvName, "tap")) {
> +                offset = strchr(src, ':');
> +                if (!offset)
> +                    goto bad_parse;
> +
> +                drvType = malloc((offset-src)+1);

  Same here. If failing a libvirt error and going to bad_parse should be
sufficient I guess.

> +                    } else if (!strcmp(offset, ":disk")) {
> +                        /* defualt anyway */

  /* default */ :-)
> +            } else if ((drvName == NULL) &&
> +                       (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
> +                drvName = xmlGetProp(cur, BAD_CAST "name");

  testing for NULL would be good, if the attribute is missing we should
not crash

> +                if (!strcmp((const char *)drvName, "tap"))
> +                    drvType = xmlGetProp(cur, BAD_CAST "type");
>              } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
>                  ro = 1;
>              }
>  <at>  <at>  -972,14 +979,14  <at>  <at> 
>      /* Xend (all versions) put the floppy device config
>       * under the hvm (image (os)) block
>       */
> -    if (hvm && 
> +    if (hvm &&
>          device &&
>          !strcmp((const char *)device, "floppy")) {
>          return 0;
>      }
>  
>      /* Xend <= 3.0.2 doesn't include cdrom config here */
> -    if (hvm && 
> +    if (hvm &&
>          device &&
>          !strcmp((const char *)device, "cdrom")) {
>          if (xendConfigVersion == 1)
>  <at>  <at>  -990,7 +997,14  <at>  <at> 
>  
>  
>      virBufferAdd(buf, "(device ", 8);
> -    virBufferAdd(buf, "(vbd ", 5);
> +    /* Why, oh why did this need to change as well as the
> +       specifying tap in the (uname..) block ??!!?! Crazy
> +       Xen formats :-( */

  I undestand the frustration, but 3 years from now the comment will lack
relevance, unless you give the full picture :-)

    thanks !

Daniel

--

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard <at> redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

Daniel P. Berrange | 6 Oct 2006 12:57
Picon
Favicon

Re: Supporting new Xen 3.0.3 blktap backend for file devices

On Fri, Oct 06, 2006 at 04:44:17AM -0400, Daniel Veillard wrote:
> On Thu, Oct 05, 2006 at 07:23:15PM +0100, Daniel P. Berrange wrote:
> > diff -u -r1.66 xend_internal.c
> > --- src/xend_internal.c	29 Sep 2006 12:00:58 -0000	1.66
> > +++ src/xend_internal.c	5 Oct 2006 18:12:33 -0000
> >  <at>  <at>  -1499,7 +1499,7  <at>  <at> 
> >  	for (i = 0, j = 0;(i < 32) && (tmp[j] != 0);j++) {
> >  	    if (((tmp[j] >= '0') && (tmp[j] <= '9')) ||
> >  	        ((tmp[j] >= 'a') && (tmp[j] <= 'f')))
> > -		compact[i++] = tmp[j];
> > +            compact[i++] = tmp[j];
> 
>   maybe we should just add the full set of { } for the innner constructs 
> too if reformatting.

Will do.

> >  	    else if ((tmp[j] >= 'A') && (tmp[j] <= 'F'))
> >  	        compact[i++] = tmp[j] + 'a' - 'A';
> >  	}
> >  <at>  <at>  -1546,95 +1546,116  <at>  <at> 
> > +            drvName = malloc((offset-src)+1);
> 
>   I agree that if we OOM there it's gonna be messy anyway but let's catch
> NULL returns on allocs as much as possible

Don't know what I was thinking. Clearly I should be checking malloc() calls
for failure. Will fix.

> > +            strncpy(drvName, src, (offset-src));
> > +            drvName[offset-src] = '\0';
> > +
> > +            src = offset + 1;
> > +
> > +            if (!strcmp(drvName, "tap")) {
> > +                offset = strchr(src, ':');
> > +                if (!offset)
> > +                    goto bad_parse;
> > +
> > +                drvType = malloc((offset-src)+1);
> 
>   Same here. If failing a libvirt error and going to bad_parse should be
> sufficient I guess.

Will fix.

> 
> > +                    } else if (!strcmp(offset, ":disk")) {
> > +                        /* defualt anyway */
> 
>   /* default */ :-)
> > +            } else if ((drvName == NULL) &&
> > +                       (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
> > +                drvName = xmlGetProp(cur, BAD_CAST "name");
> 
>   testing for NULL would be good, if the attribute is missing we should
> not crash

Yes, its fine for this to be NULL - i guess I was just lucky that this
next strcmp didn't crash when passed NULL. Will fix.

> > +                if (!strcmp((const char *)drvName, "tap"))
> > +                    drvType = xmlGetProp(cur, BAD_CAST "type");
> >              } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
> >                  ro = 1;
> >              }

Regards,
Dan.
--

-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 


Gmane