Re: [PATCH] Allow blktap to be able to be booted as system volume for PV-on-HVM

["Daniel P. Berrange" <berrange@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

On Fri, Jul 06, 2007 at 07:50:06PM +0900, Kasai Takanori wrote:
> Hi All,
> 
> We were testing the PV driver on the HVM domain.
> When blktap was booting system volume, PV-on-HVM domain was not able to be 
> started.
> 
> The configuration file is specified as follows.
> disk = [ 'tap:aio:/xen/test/rhel5ga_full.img,hda,w' ]
> 
> The error occurred by the initialization of system volume in qemu-dm.
> 
> qemu: could not open hard disk image 'aio:/xen/test/rhel5ga_full.img'
> 
> It is because "aio:" is added to the head of params in xenstore.
> However, qemu-dm open device by params.
> 
> This patch corrected the problem of params on the qemu-dm.

That's not safe enough since it always looks for a ':' regardless of
whether the driver is 'tap' or not. It also doesn't take account of
fact that the device may be named 'xvda' rather than 'hda'. 

I posted patches to address this more completely here:

http://lists.xensource.com/archives/html/xen-devel/2007-06/msg01021.html

I'm attaching them again for convenience

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  -=| 

[-- Attachment #2: xen-revert-phantom.patch --]
[-- Type: text/plain, Size: 12359 bytes --]

diff -r c0b0974fb055 tools/ioemu/xenstore.c
--- a/tools/ioemu/xenstore.c	Fri May 18 16:59:32 2007 +0100
+++ b/tools/ioemu/xenstore.c	Fri Jun 29 15:48:53 2007 -0400
@@ -10,7 +10,6 @@
 
 #include "vl.h"
 #include "block_int.h"
-#include <unistd.h>
 #include <sys/ipc.h>
 #include <sys/shm.h>
 #include <sys/types.h>
@@ -61,28 +60,11 @@ void xenstore_check_new_media_present(in
     qemu_mod_timer(insert_timer, qemu_get_clock(rt_clock) + timeout);
 }
 
-static void waitForDevice(char *fn)
-{ 
-    struct stat sbuf;
-    int status;
-    int uwait = UWAIT_MAX;
-
-    do {
-        status = stat(fn, &sbuf);
-        if (!status) break;
-        usleep(UWAIT);
-        uwait -= UWAIT;
-    } while (uwait > 0);
-
-    return;
-}
-
 void xenstore_parse_domain_config(int domid)
 {
     char **e = NULL;
     char *buf = NULL, *path;
-    char *fpath = NULL, *bpath = NULL,
-        *dev = NULL, *params = NULL, *type = NULL;
+    char *bpath = NULL, *dev = NULL, *params = NULL, *type = NULL;
     int i, is_scsi;
     unsigned int len, num, hd_index;
 
@@ -140,36 +122,12 @@ void xenstore_parse_domain_config(int do
         params = xs_read(xsh, XBT_NULL, buf, &len);
         if (params == NULL)
             continue;
-        /* 
-         * check if device has a phantom vbd; the phantom is hooked
-         * to the frontend device (for ease of cleanup), so lookup 
-         * the frontend device, and see if there is a phantom_vbd
-         * if there is, we will use resolution as the filename
-         */
-        if (pasprintf(&buf, "%s/device/vbd/%s/phantom_vbd", path, e[i]) == -1)
-            continue;
-        free(fpath);
-        fpath = xs_read(xsh, XBT_NULL, buf, &len);
-        if (fpath) {
-            if (pasprintf(&buf, "%s/dev", fpath) == -1)
-                continue;
-            free(params);
-            params = xs_read(xsh, XBT_NULL, buf , &len);
-            if (params) {
-                /* 
-                 * wait for device, on timeout silently fail because we will 
-                 * fail to open below
-                 */
-                waitForDevice(params);
-            }
-        }
 
         bs_table[hd_index + (is_scsi ? MAX_DISKS : 0)] = bdrv_new(dev);
         /* check if it is a cdrom */
         if (type && !strcmp(type, "cdrom")) {
             bdrv_set_type_hint(bs_table[hd_index], BDRV_TYPE_CDROM);
-            if (pasprintf(&buf, "%s/params", bpath) != -1)
-                xs_watch(xsh, buf, dev);
+	    xs_watch(xsh, buf, dev);
         }
         /* open device now if media present */
         if (params[0]) {
diff -r c0b0974fb055 tools/python/xen/xend/XendConfig.py
--- a/tools/python/xen/xend/XendConfig.py	Fri May 18 16:59:32 2007 +0100
+++ b/tools/python/xen/xend/XendConfig.py	Fri Jun 29 15:37:43 2007 -0400
@@ -1126,47 +1126,6 @@ class XendConfig(dict):
         # no valid device to add
         return ''
 
-    def phantom_device_add(self, dev_type, cfg_xenapi = None,
-                   target = None):
-        """Add a phantom tap device configuration in XenAPI struct format.
-        """
-
-        if target == None:
-            target = self
-        
-        if dev_type not in XendDevices.valid_devices() and \
-           dev_type not in XendDevices.pseudo_devices():        
-            raise XendConfigError("XendConfig: %s not a valid device type" %
-                            dev_type)
-
-        if cfg_xenapi == None:
-            raise XendConfigError("XendConfig: device_add requires some "
-                                  "config.")
-
-        if cfg_xenapi:
-            log.debug("XendConfig.phantom_device_add: %s" % str(cfg_xenapi))
- 
-        if cfg_xenapi:
-            dev_info = {}            
-            if dev_type in ('vbd', 'tap'):
-                if dev_type == 'vbd':
-                    dev_info['uname'] = cfg_xenapi.get('image', '')
-                    dev_info['dev'] = '%s:disk' % cfg_xenapi.get('device')
-                elif dev_type == 'tap':
-                    if cfg_xenapi.get('image').find('tap:') == -1:
-                        dev_info['uname'] = 'tap:qcow:%s' % cfg_xenapi.get('image')
-                    dev_info['dev'] =  '/dev/%s' % cfg_xenapi.get('device')
-                    dev_info['uname'] = cfg_xenapi.get('image')
-                dev_info['mode'] = cfg_xenapi.get('mode')
-                dev_info['backend'] = '0'
-                dev_uuid = cfg_xenapi.get('uuid', uuid.createString())
-                dev_info['uuid'] = dev_uuid
-                self['devices'][dev_uuid] = (dev_type, dev_info)
-                self['vbd_refs'].append(dev_uuid)
-                return dev_uuid
-
-        return ''
-
     def console_add(self, protocol, location, other_config = {}):
         dev_uuid = uuid.createString()
         if protocol == 'vt100':
diff -r c0b0974fb055 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Fri May 18 16:59:32 2007 +0100
+++ b/tools/python/xen/xend/XendDomainInfo.py	Fri Jun 29 15:37:43 2007 -0400
@@ -1623,49 +1623,14 @@ class XendDomainInfo:
     # VM Destroy
     # 
 
-    def _prepare_phantom_paths(self):
-        # get associated devices to destroy
-        # build list of phantom devices to be removed after normal devices
-        plist = []
-        if self.domid is not None:
-            from xen.xend.xenstore.xstransact import xstransact
-            t = xstransact("%s/device/vbd" % GetDomainPath(self.domid))
-            for dev in t.list():
-                backend_phantom_vbd = xstransact.Read("%s/device/vbd/%s/phantom_vbd" \
-                                      % (self.dompath, dev))
-                if backend_phantom_vbd is not None:
-                    frontend_phantom_vbd =  xstransact.Read("%s/frontend" \
-                                      % backend_phantom_vbd)
-                    plist.append(backend_phantom_vbd)
-                    plist.append(frontend_phantom_vbd)
-        return plist
-
-    def _cleanup_phantom_devs(self, plist):
-        # remove phantom devices
-        if not plist == []:
-            time.sleep(2)
-        for paths in plist:
-            if paths.find('backend') != -1:
-                from xen.xend.server import DevController
-                # Modify online status /before/ updating state (latter is watched by
-                # drivers, so this ordering avoids a race).
-                xstransact.Write(paths, 'online', "0")
-                xstransact.Write(paths, 'state', str(DevController.xenbusState['Closing']))
-            # force
-            xstransact.Remove(paths)
-
     def destroy(self):
         """Cleanup VM and destroy domain.  Nothrow guarantee."""
 
         log.debug("XendDomainInfo.destroy: domid=%s", str(self.domid))
-
-        paths = self._prepare_phantom_paths()
 
         self._cleanupVm()
         if self.dompath is not None:
             self.destroyDomain()
-
-        self._cleanup_phantom_devs(paths)
 
         if "transient" in self.info["other_config"] \
            and bool(self.info["other_config"]["transient"]):
@@ -1675,8 +1640,6 @@ class XendDomainInfo:
 
     def destroyDomain(self):
         log.debug("XendDomainInfo.destroyDomain(%s)", str(self.domid))
-
-        paths = self._prepare_phantom_paths()
 
         try:
             if self.domid is not None:
@@ -1692,7 +1655,7 @@ class XendDomainInfo:
         XendDomain.instance().remove_domain(self)
 
         self.cleanupDomain()
-        self._cleanup_phantom_devs(paths)
+
 
 
     def resumeDomain(self):
@@ -2403,25 +2366,6 @@ class XendDomainInfo:
             
         return dev_uuid
 
-    def create_phantom_vbd_with_vdi(self, xenapi_vbd, vdi_image_path):
-        """Create a VBD using a VDI from XendStorageRepository.
-
-        @param xenapi_vbd: vbd struct from the Xen API
-        @param vdi_image_path: VDI UUID
-        @rtype: string
-        @return: uuid of the device
-        """
-        xenapi_vbd['image'] = vdi_image_path
-        dev_uuid = self.info.phantom_device_add('tap', cfg_xenapi = xenapi_vbd)
-        if not dev_uuid:
-            raise XendError('Failed to create device')
-
-        if self._stateGet() == XEN_API_VM_POWER_STATE_RUNNING:
-            _, config = self.info['devices'][dev_uuid]
-            config['devid'] = self.getDeviceController('tap').createDevice(config)
-
-        return config['devid']
-
     def create_vif(self, xenapi_vif):
         """Create VIF device from the passed struct in Xen API format.
 
diff -r c0b0974fb055 tools/python/xen/xend/server/BlktapController.py
--- a/tools/python/xen/xend/server/BlktapController.py	Fri May 18 16:59:32 2007 +0100
+++ b/tools/python/xen/xend/server/BlktapController.py	Fri Jun 29 15:37:43 2007 -0400
@@ -2,10 +2,7 @@
 
 
 from xen.xend.server.blkif import BlkifController
-from xen.xend.XendLogging import log
 
-phantomDev = 0;
-phantomId = 0;
 
 class BlktapController(BlkifController):
     def __init__(self, vm):
@@ -15,62 +12,3 @@ class BlktapController(BlkifController):
         """@see DevController#frontendRoot"""
         
         return "%s/device/vbd" % self.vm.getDomainPath()
-
-    def getDeviceDetails(self, config):
-        (devid, back, front) = BlkifController.getDeviceDetails(self, config)
-
-        phantomDevid = 0
-        wrapped = False
-
-        try:
-            imagetype = self.vm.info['image']['type']
-        except:
-            imagetype = ""
-
-        if imagetype == 'hvm':
-            tdevname = back['dev']
-            index = ['c', 'd', 'e', 'f', 'g', 'h', 'i', \
-                     'j', 'l', 'm', 'n', 'o', 'p']
-            while True:
-                global phantomDev
-                global phantomId
-                import os, stat
-
-                phantomId = phantomId + 1
-                if phantomId == 16:
-                    if index[phantomDev] == index[-1]:
-                        if wrapped:
-                            raise VmError(" No loopback block \
-                                       devices are available. ")
-                        wrapped = True
-                        phantomDev = 0
-                    else:
-                        phantomDev = phantomDev + 1
-                    phantomId = 1
-                devname = 'xvd%s%d' % (index[phantomDev], phantomId)
-                try:
-                    info = os.stat('/dev/%s' % devname)
-                except:
-                    break
-
-            vbd = { 'mode': 'w', 'device': devname }
-            fn = 'tap:%s' % back['params']
-
-            # recurse ... by creating the vbd, then fallthrough
-            # and finish creating the original device
-
-            from xen.xend import XendDomain
-            dom0 = XendDomain.instance().privilegedDomain()
-            phantomDevid = dom0.create_phantom_vbd_with_vdi(vbd, fn)
-            # we need to wait for this device at a higher level
-            # the vbd that gets created will have a link to us
-            # and will let them do it there
-
-        # add a hook to point to the phantom device,
-        # root path is always the same (dom0 tap)
-        if phantomDevid != 0:
-            front['phantom_vbd'] = '/local/domain/0/backend/tap/0/%s' \
-                                   % str(phantomDevid)
-
-        return (devid, back, front)
-
diff -r c0b0974fb055 tools/python/xen/xend/server/DevController.py
--- a/tools/python/xen/xend/server/DevController.py	Fri May 18 16:59:32 2007 +0100
+++ b/tools/python/xen/xend/server/DevController.py	Fri Jun 29 15:37:43 2007 -0400
@@ -474,19 +474,6 @@ class DevController:
     def waitForBackend(self, devid):
 
         frontpath = self.frontendPath(devid)
-        # lookup a phantom 
-        phantomPath = xstransact.Read(frontpath, 'phantom_vbd')
-        if phantomPath is not None:
-            log.debug("Waiting for %s's phantom %s.", devid, phantomPath)
-            statusPath = phantomPath + '/' + HOTPLUG_STATUS_NODE
-            ev = Event()
-            result = { 'status': Timeout }
-            xswatch(statusPath, hotplugStatusCallback, ev, result)
-            ev.wait(DEVICE_CREATE_TIMEOUT)
-            err = xstransact.Read(statusPath, HOTPLUG_ERROR_NODE)
-            if result['status'] != 'Connected':
-                return (result['status'], err)
-            
         backpath = xstransact.Read(frontpath, "backend")
 
 

[-- Attachment #3: qemu-blktap.patch --]
[-- Type: text/plain, Size: 1557 bytes --]

--- xen-3.1.0-src.new/tools/ioemu/xenstore.c	2007-05-18 10:45:21.000000000 -0400
+++ xen-3.1.0-src/tools/ioemu/xenstore.c	2007-06-22 13:48:55.000000000 -0400
@@ -83,7 +83,7 @@ void xenstore_parse_domain_config(int do
     char *buf = NULL, *path;
     char *fpath = NULL, *bpath = NULL,
         *dev = NULL, *params = NULL, *type = NULL;
-    int i, is_scsi;
+    int i, is_scsi, is_tap = 0;
     unsigned int len, num, hd_index;
 
     for(i = 0; i < MAX_DISKS + MAX_SCSI_DISKS; i++)
@@ -123,6 +123,14 @@ void xenstore_parse_domain_config(int do
         dev = xs_read(xsh, XBT_NULL, buf, &len);
         if (dev == NULL)
             continue;
+	/* Force xvdN to look like  hdN */
+	if (!strncmp(dev, "xvd", 3)) {
+	  memmove(dev, dev+1, strlen(dev));
+	  dev[0] = 'h';
+	  dev[1] = 'd';
+	  is_tap = 1;
+	  fprintf(stderr, "Got blktap '%s'\n", dev);
+	}
         is_scsi = !strncmp(dev, "sd", 2);
         if ((strncmp(dev, "hd", 2) && !is_scsi) || strlen(dev) != 3 )
             continue;
@@ -140,6 +148,14 @@ void xenstore_parse_domain_config(int do
         params = xs_read(xsh, XBT_NULL, buf, &len);
         if (params == NULL)
             continue;
+	/* Strip off blktap sub-type prefix aio: etc */
+	if (is_tap) {
+	  char *offset = strchr(params, ':');
+	  if (!offset)
+	    continue;
+	  memmove(params, offset+1, strlen(offset+1)+1);
+	  fprintf(stderr, "Got params '%s'\n", params);
+	}
         /* 
          * check if device has a phantom vbd; the phantom is hooked
          * to the frontend device (for ease of cleanup), so lookup 

[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel