xl create PV guest with qcow/qcow2 disk images fail

xl create PV guest with qcow/qcow2 disk images fail

[Chun Yan Liu]

[-- Attachment #1.1: Type: text/plain, Size: 645 bytes --]

Hi, List,

I'm trying xl create a pv guest with qcow/qcow2 image, it always fails at libxl_device_disk_local_attach. 
#xl create pv_config_file
libxl: error: libxl.c:1119:libxl_device_disk_local_attach: cannot locally attach a qdisk image if the format is not raw
libxl: error: libxl_create.c:467:do_domain_create: failed to run bootloader: -3

disk configuration is:
disk=[ 'tap:qcow2:/var/lib/xen/images/sles11pv/disk0.qcow2,xvda,w', ]

Is there any way to make it work?

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





[-- Attachment #1.2: Type: text/html, Size: 993 bytes --]

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

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

Re: xl create PV guest with qcow/qcow2 disk images fail

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 845 bytes --]

On Fri, 2011-10-14 at 02:25 -0600, Chun Yan Liu wrote:
> Hi, List,
>
Hi,

> disk configuration is:
> disk=[ 'tap:qcow2:/var/lib/xen/images/sles11pv/disk0.qcow2,xvda,w', ]
>
> Is there any way to make it work?
>
I'm using this and it's working for me:

disk = [ 'format=qcow2, vdev=hda, access=rw, target=/root/xen/VMs/debian_squeeze_amd64_standard.qcow2' ]

For more info you can check here
"docs/misc/xl-disk-configuration.txt" (if you happen to have the sources
handy).

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

Re: xl create PV guest with qcow/qcow2 disk images fail

[Ian Campbell]

On Fri, 2011-10-14 at 09:25 +0100, Chun Yan Liu wrote:
> Hi, List,
> 
> I'm trying xl create a pv guest with qcow/qcow2 image, it always fails
> at libxl_device_disk_local_attach. 
> #xl create pv_config_file
> libxl: error: libxl.c:1119:libxl_device_disk_local_attach: cannot
> locally attach a qdisk image if the format is not raw
> libxl: error: libxl_create.c:467:do_domain_create: failed to run
> bootloader: -3

Unfortunately non-raw disks are currently incompatible with using pygrub
in xl.

I expect that the correct solution would be to patch
libxl_device_disk_local_attach() to start qemu-nbd and make that device
available in the local domain.

Ian.

> 
> disk configuration is:
> disk=[ 'tap:qcow2:/var/lib/xen/images/sles11pv/disk0.qcow2,xvda,w', ]
> 
> Is there any way to make it work?
> 
> Thanks,
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
> 
> 

Re: xl create PV guest with qcow/qcow2 disk images fail

[Stefano Stabellini]

On Fri, 14 Oct 2011, Chun Yan Liu wrote:
> Hi, List,
> 
> I'm trying xl create a pv guest with qcow/qcow2 image, it always fails at libxl_device_disk_local_attach.
> #xl create pv_config_file
> libxl: error: libxl.c:1119:libxl_device_disk_local_attach: cannot locally attach a qdisk image if the format is not raw
> libxl: error: libxl_create.c:467:do_domain_create: failed to run bootloader: -3
> 
> disk configuration is:
> disk=[ 'tap:qcow2:/var/lib/xen/images/sles11pv/disk0.qcow2,xvda,w', ]
> 
> Is there any way to make it work?

This is a PV guest configured with pygrub, correct?
If so, qcow/qcow2 are not supported in this scenario.

You could:

- avoid using pygrub (specify the kernel manually) and keep using qcow/qcow2;
- switch to raw disks and keep using pygrub;
- install a Linux kernel that support blktap2 (like the XCP kernel, see
  http://wiki.xen.org/xenwiki/XenDom0Kernels) and switch to VHD format.

Re: xl create PV guest with qcow/qcow2 disk images fail

[Stefano Stabellini]

On Wed, 19 Oct 2011, Stefano Stabellini wrote:
> On Fri, 14 Oct 2011, Chun Yan Liu wrote:
> > Hi, List,
> > 
> > I'm trying xl create a pv guest with qcow/qcow2 image, it always fails at libxl_device_disk_local_attach.
> > #xl create pv_config_file
> > libxl: error: libxl.c:1119:libxl_device_disk_local_attach: cannot locally attach a qdisk image if the format is not raw
> > libxl: error: libxl_create.c:467:do_domain_create: failed to run bootloader: -3
> > 
> > disk configuration is:
> > disk=[ 'tap:qcow2:/var/lib/xen/images/sles11pv/disk0.qcow2,xvda,w', ]
> > 
> > Is there any way to make it work?
> 
> This is a PV guest configured with pygrub, correct?
> If so, qcow/qcow2 are not supported in this scenario.
> 
> You could:
> 
> - avoid using pygrub (specify the kernel manually) and keep using qcow/qcow2;
> - switch to raw disks and keep using pygrub;
> - install a Linux kernel that support blktap2 (like the XCP kernel, see
>   http://wiki.xen.org/xenwiki/XenDom0Kernels) and switch to VHD format.
> 

The way to make it work would be to call qemu-nbd and nbd-client from xl
so that a /dev/nbd0 comes up in dom0 and pygrub can use it to extract
the kernel and initrd from the qcow2 image.

Re: xl create PV guest with qcow/qcow2 disk images fail

[Fajar A. Nugraha]

On Wed, Oct 19, 2011 at 5:55 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>> This is a PV guest configured with pygrub, correct?
>> If so, qcow/qcow2 are not supported in this scenario.
>>
>> You could:
>>
>> - avoid using pygrub (specify the kernel manually) and keep using qcow/qcow2;
>> - switch to raw disks and keep using pygrub;
>> - install a Linux kernel that support blktap2 (like the XCP kernel, see
>>   http://wiki.xen.org/xenwiki/XenDom0Kernels) and switch to VHD format.
>>
>
> The way to make it work would be to call qemu-nbd and nbd-client from xl
> so that a /dev/nbd0 comes up in dom0 and pygrub can use it to extract
> the kernel and initrd from the qcow2 image.

would pv-grub work? If yes, it would give better performance compared
to nbd workaround.

-- 
Fajar

Re: xl create PV guest with qcow/qcow2 disk images fail

[Stefano Stabellini]

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

On Wed, 19 Oct 2011, Fajar A. Nugraha wrote:
> On Wed, Oct 19, 2011 at 5:55 PM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> >> This is a PV guest configured with pygrub, correct?
> >> If so, qcow/qcow2 are not supported in this scenario.
> >>
> >> You could:
> >>
> >> - avoid using pygrub (specify the kernel manually) and keep using qcow/qcow2;
> >> - switch to raw disks and keep using pygrub;
> >> - install a Linux kernel that support blktap2 (like the XCP kernel, see
> >>   http://wiki.xen.org/xenwiki/XenDom0Kernels) and switch to VHD format.
> >>
> >
> > The way to make it work would be to call qemu-nbd and nbd-client from xl
> > so that a /dev/nbd0 comes up in dom0 and pygrub can use it to extract
> > the kernel and initrd from the qcow2 image.
> 
> would pv-grub work? If yes, it would give better performance compared
> to nbd workaround.

Yes, it should. That would be the other alternative.

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

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

Re: xl create PV guest with qcow/qcow2 disk images fail

[Chun Yan Liu]

[-- Attachment #1.1: Type: text/plain, Size: 5697 bytes --]


Thank you all. Have tested pv-grub and qemu-nbd trick. Both work. 
Following is the test patch that starts qemu-nbd to mount a non-raw qdisk in domain0, so that it can work with qcow/qcow2 disk image and using pygrub. I don't know if we need such a patch, or prefer to ask user to use pv-grub instead. Just post here for any chance of use. Thanks. 
  
Patch description: start qemu-nbd to mount non-raw qdisk in dom0 so that xl can create PV guest with qcow/qcow2 disk image and using pygrub. 
Signed-off-by: Chunyan Liu <cyliu@suse.com> 

diff -r b4cf57bbc3fb tools/libxl/libxl.c 
--- a/tools/libxl/libxl.cThu Oct 20 15:24:46 2011 +0800 
+++ b/tools/libxl/libxl.cThu Oct 20 15:48:45 2011 +0800 
@@ -1078,12 +1078,41 @@ 
     return rc; 
 } 
  
+static char * nbd_mount_disk(libxl_device_disk *disk) 
+{ 
+    int i; 
+    int nbds_max = 16; 
+    char *nbd_dev, *cmd; 
+    char *ret = NULL; 
+ 
+    for (i = 0; i < nbds_max; i++) { 
+        asprintf(&nbd_dev,"/dev/nbd%d", i); 
+        asprintf(&cmd, "qemu-nbd -c %s %s", nbd_dev, disk->pdev_path); 
+        if (system(cmd) == 0) { 
+            ret = strdup(nbd_dev); 
+            break; 
+        } 
+    } 
+ 
+    return ret; 
+} 
+ 
+static int nbd_unmount_disk(char *diskpath) { 
+    char *cmd; 
+    asprintf(&cmd, "qemu-nbd -d %s", diskpath); 
+    if (system(cmd) == 0) 
+        return 0; 
+    else 
+        return ERROR_FAIL; 
+} 
+ 
 char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk) 
 { 
     libxl__gc gc = LIBXL_INIT_GC(ctx); 
     char *dev = NULL; 
     char *ret = NULL; 
     int rc; 
+    char *mdev = NULL; 
  
     rc = libxl__device_disk_set_backend(&gc, disk); 
     if (rc) goto out; 
@@ -1118,8 +1147,12 @@ 
             break; 
         case LIBXL_DISK_BACKEND_QDISK: 
             if (disk->format != LIBXL_DISK_FORMAT_RAW) { 
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally" 
-                           " attach a qdisk image if the format is not raw"); 
+                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching a non-raw qdisk image to domain 0\n"); 
+                mdev = nbd_mount_disk(disk); 
+                if (mdev) 
+                    dev = mdev; 
+                else 
+                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "fail to mount image with qemu-nbd"); 
                 break; 
             } 
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n", 
@@ -1135,11 +1168,13 @@ 
  out: 
     if (dev != NULL) 
         ret = strdup(dev); 
+    if (mdev) 
+        free(mdev); 
     libxl__free_all(&gc); 
     return ret; 
 } 
  
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk) 
+int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk, char *diskpath) 
 { 
     /* Nothing to do for PHYSTYPE_PHY. */ 
  
@@ -1147,6 +1182,19 @@ 
      * For other device types assume that the blktap2 process is 
      * needed by the soon to be started domain and do nothing. 
      */ 
+    int ret; 
+ 
+    switch (disk->backend) { 
+        case LIBXL_DISK_BACKEND_QDISK: 
+            if (disk->format != LIBXL_DISK_FORMAT_RAW) { 
+                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Locally detach a non-raw " 
+                    "qdisk image"); 
+                ret = nbd_unmount_disk(diskpath); 
+                return ret; 
+            } 
+        default: 
+            break; 
+    } 
  
     return 0; 
 } 
diff -r b4cf57bbc3fb tools/libxl/libxl.h 
--- a/tools/libxl/libxl.hThu Oct 20 15:24:46 2011 +0800 
+++ b/tools/libxl/libxl.hThu Oct 20 15:48:45 2011 +0800 
@@ -390,7 +390,7 @@ 
  * Make a disk available in this domain. Returns path to a device. 
  */ 
 char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk); 
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk); 
+int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk, char *diskpath); 
  
 int libxl_device_nic_init(libxl_device_nic *nic, int dev_num); 
 int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic); 
diff -r b4cf57bbc3fb tools/libxl/libxl_bootloader.c 
--- a/tools/libxl/libxl_bootloader.cThu Oct 20 15:24:46 2011 +0800 
+++ b/tools/libxl/libxl_bootloader.cThu Oct 20 15:48:45 2011 +0800 
@@ -424,7 +424,7 @@ 
     rc = 0; 
 out_close: 
     if (diskpath) { 
-        libxl_device_disk_local_detach(ctx, disk); 
+        libxl_device_disk_local_detach(ctx, disk, diskpath); 
         free(diskpath); 
     } 
     if (fifo_fd > -1) 

>>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 10/19/2011 9:40 PM >>>
On Wed, 19 Oct 2011, Fajar A. Nugraha wrote:
> On Wed, Oct 19, 2011 at 5:55 PM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> >> This is a PV guest configured with pygrub, correct?
> >> If so, qcow/qcow2 are not supported in this scenario.
> >>
> >> You could:
> >>
> >> - avoid using pygrub (specify the kernel manually) and keep using qcow/qcow2;
> >> - switch to raw disks and keep using pygrub;
> >> - install a Linux kernel that support blktap2 (like the XCP kernel, see
> >>   http://wiki.xen.org/xenwiki/XenDom0Kernels) and switch to VHD format.
> >>
> >
> > The way to make it work would be to call qemu-nbd and nbd-client from xl
> > so that a /dev/nbd0 comes up in dom0 and pygrub can use it to extract
> > the kernel and initrd from the qcow2 image.
>
> would pv-grub work? If yes, it would give better performance compared
> to nbd workaround.

Yes, it should. That would be the other alternative.

[-- Attachment #1.2: Type: text/html, Size: 23131 bytes --]

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

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

Re: xl create PV guest with qcow/qcow2 disk images fail

[Chun Yan Liu]

[-- Attachment #1.1: Type: text/plain, Size: 5697 bytes --]


Thank you all. Have tested pv-grub and qemu-nbd trick. Both work. 
Following is the test patch that starts qemu-nbd to mount a non-raw qdisk in domain0, so that it can work with qcow/qcow2 disk image and using pygrub. I don't know if we need such a patch, or prefer to ask user to use pv-grub instead. Just post here for any chance of use. Thanks. 
  
Patch description: start qemu-nbd to mount non-raw qdisk in dom0 so that xl can create PV guest with qcow/qcow2 disk image and using pygrub. 
Signed-off-by: Chunyan Liu <cyliu@suse.com> 

diff -r b4cf57bbc3fb tools/libxl/libxl.c 
--- a/tools/libxl/libxl.cThu Oct 20 15:24:46 2011 +0800 
+++ b/tools/libxl/libxl.cThu Oct 20 15:48:45 2011 +0800 
@@ -1078,12 +1078,41 @@ 
     return rc; 
 } 
  
+static char * nbd_mount_disk(libxl_device_disk *disk) 
+{ 
+    int i; 
+    int nbds_max = 16; 
+    char *nbd_dev, *cmd; 
+    char *ret = NULL; 
+ 
+    for (i = 0; i < nbds_max; i++) { 
+        asprintf(&nbd_dev,"/dev/nbd%d", i); 
+        asprintf(&cmd, "qemu-nbd -c %s %s", nbd_dev, disk->pdev_path); 
+        if (system(cmd) == 0) { 
+            ret = strdup(nbd_dev); 
+            break; 
+        } 
+    } 
+ 
+    return ret; 
+} 
+ 
+static int nbd_unmount_disk(char *diskpath) { 
+    char *cmd; 
+    asprintf(&cmd, "qemu-nbd -d %s", diskpath); 
+    if (system(cmd) == 0) 
+        return 0; 
+    else 
+        return ERROR_FAIL; 
+} 
+ 
 char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk) 
 { 
     libxl__gc gc = LIBXL_INIT_GC(ctx); 
     char *dev = NULL; 
     char *ret = NULL; 
     int rc; 
+    char *mdev = NULL; 
  
     rc = libxl__device_disk_set_backend(&gc, disk); 
     if (rc) goto out; 
@@ -1118,8 +1147,12 @@ 
             break; 
         case LIBXL_DISK_BACKEND_QDISK: 
             if (disk->format != LIBXL_DISK_FORMAT_RAW) { 
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally" 
-                           " attach a qdisk image if the format is not raw"); 
+                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching a non-raw qdisk image to domain 0\n"); 
+                mdev = nbd_mount_disk(disk); 
+                if (mdev) 
+                    dev = mdev; 
+                else 
+                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "fail to mount image with qemu-nbd"); 
                 break; 
             } 
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n", 
@@ -1135,11 +1168,13 @@ 
  out: 
     if (dev != NULL) 
         ret = strdup(dev); 
+    if (mdev) 
+        free(mdev); 
     libxl__free_all(&gc); 
     return ret; 
 } 
  
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk) 
+int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk, char *diskpath) 
 { 
     /* Nothing to do for PHYSTYPE_PHY. */ 
  
@@ -1147,6 +1182,19 @@ 
      * For other device types assume that the blktap2 process is 
      * needed by the soon to be started domain and do nothing. 
      */ 
+    int ret; 
+ 
+    switch (disk->backend) { 
+        case LIBXL_DISK_BACKEND_QDISK: 
+            if (disk->format != LIBXL_DISK_FORMAT_RAW) { 
+                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Locally detach a non-raw " 
+                    "qdisk image"); 
+                ret = nbd_unmount_disk(diskpath); 
+                return ret; 
+            } 
+        default: 
+            break; 
+    } 
  
     return 0; 
 } 
diff -r b4cf57bbc3fb tools/libxl/libxl.h 
--- a/tools/libxl/libxl.hThu Oct 20 15:24:46 2011 +0800 
+++ b/tools/libxl/libxl.hThu Oct 20 15:48:45 2011 +0800 
@@ -390,7 +390,7 @@ 
  * Make a disk available in this domain. Returns path to a device. 
  */ 
 char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk); 
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk); 
+int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk, char *diskpath); 
  
 int libxl_device_nic_init(libxl_device_nic *nic, int dev_num); 
 int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic); 
diff -r b4cf57bbc3fb tools/libxl/libxl_bootloader.c 
--- a/tools/libxl/libxl_bootloader.cThu Oct 20 15:24:46 2011 +0800 
+++ b/tools/libxl/libxl_bootloader.cThu Oct 20 15:48:45 2011 +0800 
@@ -424,7 +424,7 @@ 
     rc = 0; 
 out_close: 
     if (diskpath) { 
-        libxl_device_disk_local_detach(ctx, disk); 
+        libxl_device_disk_local_detach(ctx, disk, diskpath); 
         free(diskpath); 
     } 
     if (fifo_fd > -1) 

>>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 10/19/2011 9:40 PM >>>
On Wed, 19 Oct 2011, Fajar A. Nugraha wrote:
> On Wed, Oct 19, 2011 at 5:55 PM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> >> This is a PV guest configured with pygrub, correct?
> >> If so, qcow/qcow2 are not supported in this scenario.
> >>
> >> You could:
> >>
> >> - avoid using pygrub (specify the kernel manually) and keep using qcow/qcow2;
> >> - switch to raw disks and keep using pygrub;
> >> - install a Linux kernel that support blktap2 (like the XCP kernel, see
> >>   http://wiki.xen.org/xenwiki/XenDom0Kernels) and switch to VHD format.
> >>
> >
> > The way to make it work would be to call qemu-nbd and nbd-client from xl
> > so that a /dev/nbd0 comes up in dom0 and pygrub can use it to extract
> > the kernel and initrd from the qcow2 image.
>
> would pv-grub work? If yes, it would give better performance compared
> to nbd workaround.

Yes, it should. That would be the other alternative.

[-- Attachment #1.2: Type: text/html, Size: 23131 bytes --]

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

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

Re: xl create PV guest with qcow/qcow2 disk images fail

[Stefano Stabellini]

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

On Thu, 27 Oct 2011, Chun Yan Liu wrote:
> 
> Thank you all. Have tested pv-grub and qemu-nbd trick. Both work.
> 
> Following is the test patch that starts qemu-nbd to mount a non-raw qdisk in domain0, so that it can work with qcow/qcow2
> disk image and using pygrub. I don't know if we need such a patch, or prefer to ask user to use pv-grub instead. Just post
> here for any chance of use. Thanks.
> 
>  
> 
> Patch description: start qemu-nbd to mount non-raw qdisk in dom0 so that xl can create PV guest with qcow/qcow2 disk image
> and using pygrub.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> 
> 
> diff -r b4cf57bbc3fb tools/libxl/libxl.c
> 
> --- a/tools/libxl/libxl.cThu Oct 20 15:24:46 2011 +0800
> 
> +++ b/tools/libxl/libxl.cThu Oct 20 15:48:45 2011 +0800
> 
> @@ -1078,12 +1078,41 @@
> 
>      return rc;
> 
>  }
> 
>  
> 
> +static char * nbd_mount_disk(libxl_device_disk *disk)
> 
> +{
> 
> +    int i;
> 
> +    int nbds_max = 16;
> 
> +    char *nbd_dev, *cmd;
> 
> +    char *ret = NULL;
> 
> +
> 
> +    for (i = 0; i < nbds_max; i++) {
> 
> +        asprintf(&nbd_dev,"/dev/nbd%d", i);
> 
> +        asprintf(&cmd, "qemu-nbd -c %s %s", nbd_dev, disk->pdev_path);
> 
> +        if (system(cmd) == 0) {
> 
> +            ret = strdup(nbd_dev);
> 
> +            break;
> 
> +        }
> 
> +    }

You should use fork, libxl_postfork and exec instead of system. See
xl_cmdimpl.c:autoconnect_console for example.
Also where are nbd_dev and cmd freed?


> +
> 
> +    return ret;
> 
> +}
> 
> +
> 
> +static int nbd_unmount_disk(char *diskpath) {
> 
> +    char *cmd;
> 
> +    asprintf(&cmd, "qemu-nbd -d %s", diskpath);
> 
> +    if (system(cmd) == 0)
> 
> +        return 0;
> 
> +    else
> 
> +        return ERROR_FAIL;
> 
> +}

Same here.


> +
> 
>  char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
> 
>  {
> 
>      libxl__gc gc = LIBXL_INIT_GC(ctx);
> 
>      char *dev = NULL;
> 
>      char *ret = NULL;
> 
>      int rc;
> 
> +    char *mdev = NULL;
> 
>  
> 
>      rc = libxl__device_disk_set_backend(&gc, disk);
> 
>      if (rc) goto out;
> 
> @@ -1118,8 +1147,12 @@
> 
>              break;
> 
>          case LIBXL_DISK_BACKEND_QDISK:
> 
>              if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> 
> -                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
> 
> -                           " attach a qdisk image if the format is not raw");
> 
> +                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching a non-raw qdisk image to domain 0\n");
> 
> +                mdev = nbd_mount_disk(disk);
> 
> +                if (mdev)
> 
> +                    dev = mdev;
> 
> +                else
> 
> +                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "fail to mount image with qemu-nbd");
> 
>                  break;
> 
>              }
> 
>              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> 
> @@ -1135,11 +1168,13 @@
> 
>   out:
> 
>      if (dev != NULL)
> 
>          ret = strdup(dev);
> 
> +    if (mdev)
> 
> +        free(mdev);
> 
>      libxl__free_all(&gc);
> 
>      return ret;
> 
>  }
> 
>  
> 
> -int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk)
> 
> +int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk, char *diskpath)
> 
>  {
> 
>      /* Nothing to do for PHYSTYPE_PHY. */
> 
>  
> 
> @@ -1147,6 +1182,19 @@
> 
>       * For other device types assume that the blktap2 process is
> 
>       * needed by the soon to be started domain and do nothing.
> 
>       */
> 
> +    int ret;
> 
> +
> 
> +    switch (disk->backend) {
> 
> +        case LIBXL_DISK_BACKEND_QDISK:
> 
> +            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> 
> +                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Locally detach a non-raw "
> 
> +                    "qdisk image");
> 
> +                ret = nbd_unmount_disk(diskpath);
> 
> +                return ret;
> 
> +            }
> 
> +        default:
> 
> +            break;
> 
> +    }
> 
>  
> 
>      return 0;
> 
>  }
> 
> diff -r b4cf57bbc3fb tools/libxl/libxl.h
> 
> --- a/tools/libxl/libxl.hThu Oct 20 15:24:46 2011 +0800
> 
> +++ b/tools/libxl/libxl.hThu Oct 20 15:48:45 2011 +0800
> 
> @@ -390,7 +390,7 @@
> 
>   * Make a disk available in this domain. Returns path to a device.
> 
>   */
> 
>  char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk);
> 
> -int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk);
> 
> +int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk, char *diskpath);
> 
>  
> 
>  int libxl_device_nic_init(libxl_device_nic *nic, int dev_num);
> 
>  int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
> 
> diff -r b4cf57bbc3fb tools/libxl/libxl_bootloader.c
> 
> --- a/tools/libxl/libxl_bootloader.cThu Oct 20 15:24:46 2011 +0800
> 
> +++ b/tools/libxl/libxl_bootloader.cThu Oct 20 15:48:45 2011 +0800
> 
> @@ -424,7 +424,7 @@
> 
>      rc = 0;
> 
>  out_close:
> 
>      if (diskpath) {
> 
> -        libxl_device_disk_local_detach(ctx, disk);
> 
> +        libxl_device_disk_local_detach(ctx, disk, diskpath);
> 
>          free(diskpath);
> 
>      }
> 
>      if (fifo_fd > -1)
> 
> 
> >>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 10/19/2011 9:40 PM >>>
> On Wed, 19 Oct 2011, Fajar A. Nugraha wrote:
> > On Wed, Oct 19, 2011 at 5:55 PM, Stefano Stabellini
> > <stefano.stabellini@eu.citrix.com> wrote:
> > >> This is a PV guest configured with pygrub, correct?
> > >> If so, qcow/qcow2 are not supported in this scenario.
> > >>
> > >> You could:
> > >>
> > >> - avoid using pygrub (specify the kernel manually) and keep using qcow/qcow2;
> > >> - switch to raw disks and keep using pygrub;
> > >> - install a Linux kernel that support blktap2 (like the XCP kernel, see
> > >>   http://wiki.xen.org/xenwiki/XenDom0Kernels) and switch to VHD format.
> > >>
> > >
> > > The way to make it work would be to call qemu-nbd and nbd-client from xl
> > > so that a /dev/nbd0 comes up in dom0 and pygrub can use it to extract
> > > the kernel and initrd from the qcow2 image.
> >
> > would pv-grub work? If yes, it would give better performance compared
> > to nbd workaround.
> 
> Yes, it should. That would be the other alternative.
> 
> 
> 

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

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

Re: xl create PV guest with qcow/qcow2 disk images fail

[Ian Campbell]

On Thu, 2011-10-27 at 04:08 +0100, Chun Yan Liu wrote:
> Thank you all. Have tested pv-grub and qemu-nbd trick. Both work. 
> 
> Following is the test patch

I'm afraid that something has horribly mangled your patch (you can see
some of what I received quoted below). Documentation/SubmittingPatches
in the Linux source tree has a bunch of hints for various MUAs. Or you
could consider usign the "hg email" extension
(http://wiki.xen.org/xenwiki/SubmittingXenPatches has a good description
of this).

Ian.


>  that starts qemu-nbd to mount a non-raw qdisk in domain0, so that it
> can work with qcow/qcow2 disk image and using pygrub. I don't know if
> we need such a patch, or prefer to ask user to use pv-grub instead.
> Just post here for any chance of use. Thanks. 
> 
>   
> 
> Patch description: start qemu-nbd to mount non-raw qdisk in dom0 so
> that xl can create PV guest with qcow/qcow2 disk image and using
> pygrub. 
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com> 
> 
> 
> diff -r b4cf57bbc3fb tools/libxl/libxl.c 
> 
> --- a/tools/libxl/libxl.cThu Oct 20 15:24:46 2011 +0800 
> 
> +++ b/tools/libxl/libxl.cThu Oct 20 15:48:45 2011 +0800 
> 
> @@ -1078,12 +1078,41 @@ 
> 
>      return rc; 
> 
>  } 
> 
>   
> 
> +static char * nbd_mount_disk(libxl_device_disk *disk) 
> 
> +{ 
> 
> +    int i; 
> 
> +    int nbds_max = 16; 
> 
> +    char *nbd_dev, *cmd; 
> 
> +    char *ret = NULL; 
> 
> + 
> 
> +    for (i = 0; i < nbds_max; i++) { 
> 
> +        asprintf(&nbd_dev,"/dev/nbd%d", i); 
> 
> +        asprintf(&cmd, "qemu-nbd -c %s %s", nbd_dev,
> disk->pdev_path); 
> 
> +        if (system(cmd) == 0) { 
> 
> +            ret = strdup(nbd_dev); 
> 
> +            break; 
> 
> +        } 
> 
> +    } 
> 
> + 
> 
> +    return ret; 
> 
> +} 
> 
> + 
> 
> +static int nbd_unmount_disk(char *diskpath) { 
> 
> +    char *cmd; 
> 
> +    asprintf(&cmd, "qemu-nbd -d %s", diskpath); 
> 
> +    if (system(cmd) == 0) 
> 
> +        return 0; 
> 
> +    else 
> 
> +        return ERROR_FAIL; 
> 
> +} 
> 
> + 

Re: xl create PV guest with qcow/qcow2 disk images fail

[Chunyan Liu]

Start qemu-nbd to mount non-raw qdisk in dom0 so that xl can create PV guest with qcow/qcow2 disk image and using pygrub.
v2: use fork and exec instead of system(3)

Signed-off-by: Chunyan Liu <cyliu@suse.com>

diff -r b4cf57bbc3fb tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu Oct 20 15:24:46 2011 +0800
+++ b/tools/libxl/libxl.c	Fri Oct 28 20:50:36 2011 +0800
@@ -1077,6 +1077,58 @@ out_free:
     libxl__free_all(&gc);
     return rc;
 }
+static int fork_exec(char *arg0, char **args)
+{
+    pid_t pid;
+    int status;
+
+    pid = fork();
+    if (pid < 0)
+        return -1;
+    else if (pid == 0){
+        execvp(arg0, args);
+        exit(127);
+    }
+    sleep(1);    
+    while (waitpid(pid, &status, 0) < 0) {
+        if (errno != EINTR) {
+            status = -1;
+            break;
+        }
+    }
+
+    return status;
+}
+
+static char * nbd_mount_disk(libxl__gc *gc, libxl_device_disk *disk)
+{
+    int i;
+    int nbds_max = 16;
+    char *nbd_dev = NULL;
+    char *args[] = {"qemu-nbd","-c",NULL,NULL,NULL};
+    char *ret = NULL;
+
+    for (i = 0; i < nbds_max; i++) {
+        nbd_dev = libxl__sprintf(gc, "/dev/nbd%d", i);
+        args[2] = libxl__sprintf(gc, "%s", nbd_dev);
+        args[3] = libxl__sprintf(gc, "%s", disk->pdev_path);
+        if (fork_exec(args[0], args) == 0) {
+            ret = strdup(nbd_dev);
+            break;
+        }
+    }
+
+    return ret;
+}
+
+static int nbd_unmount_disk(libxl__gc *gc, char *diskpath) {
+    char *args[] = {"qemu-nbd","-d",NULL,NULL};
+    args[2] = libxl__sprintf(gc, "%s", diskpath);
+    if (fork_exec(args[0], args))
+        return 0;
+    else
+        return ERROR_FAIL;
+}
 
 char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
 {
@@ -1084,6 +1136,7 @@ char * libxl_device_disk_local_attach(li
     char *dev = NULL;
     char *ret = NULL;
     int rc;
+    char *mdev = NULL;
 
     rc = libxl__device_disk_set_backend(&gc, disk);
     if (rc) goto out;
@@ -1118,8 +1171,12 @@ char * libxl_device_disk_local_attach(li
             break;
         case LIBXL_DISK_BACKEND_QDISK:
             if (disk->format != LIBXL_DISK_FORMAT_RAW) {
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
-                           " attach a qdisk image if the format is not raw");
+                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching a non-raw qdisk image to domain 0\n");
+                mdev = nbd_mount_disk(&gc, disk);
+                if (mdev)
+                    dev = mdev;
+                else
+                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "fail to mount image with qemu-nbd");
                 break;
             }
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
@@ -1135,11 +1192,13 @@ char * libxl_device_disk_local_attach(li
  out:
     if (dev != NULL)
         ret = strdup(dev);
+    if (mdev)
+        free(mdev);
     libxl__free_all(&gc);
     return ret;
 }
 
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk)
+int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk, char *diskpath)
 {
     /* Nothing to do for PHYSTYPE_PHY. */
 
@@ -1147,7 +1206,22 @@ int libxl_device_disk_local_detach(libxl
      * For other device types assume that the blktap2 process is
      * needed by the soon to be started domain and do nothing.
      */
+    libxl__gc gc = LIBXL_INIT_GC(ctx);
+    int ret;
 
+    switch (disk->backend) {
+        case LIBXL_DISK_BACKEND_QDISK:
+            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
+                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Locally detach a non-raw "
+                    "qdisk image");
+                ret = nbd_unmount_disk(&gc, diskpath);
+                return ret;
+            }
+        default:
+            break;
+    }
+
+    libxl__free_all(&gc);
     return 0;
 }
 
diff -r b4cf57bbc3fb tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Thu Oct 20 15:24:46 2011 +0800
+++ b/tools/libxl/libxl.h	Fri Oct 28 20:50:36 2011 +0800
@@ -390,7 +390,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
  * Make a disk available in this domain. Returns path to a device.
  */
 char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk);
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk);
+int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk, char *diskpath);
 
 int libxl_device_nic_init(libxl_device_nic *nic, int dev_num);
 int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
diff -r b4cf57bbc3fb tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c	Thu Oct 20 15:24:46 2011 +0800
+++ b/tools/libxl/libxl_bootloader.c	Fri Oct 28 20:50:36 2011 +0800
@@ -424,7 +424,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
     rc = 0;
 out_close:
     if (diskpath) {
-        libxl_device_disk_local_detach(ctx, disk);
+        libxl_device_disk_local_detach(ctx, disk, diskpath);
         free(diskpath);
     }
     if (fifo_fd > -1)

Re: xl create PV guest with qcow/qcow2 disk images fail

[Chun Yan Liu]

[-- Attachment #1.1: Type: text/plain, Size: 5450 bytes --]


Stefano, could you review the revised patch and share your comments? Thanks. 

>>> Chunyan Liu <cyliu@suse.com> 10/28/2011 9:27 PM >>>
Start qemu-nbd to mount non-raw qdisk in dom0 so that xl can create PV guest with qcow/qcow2 disk image and using pygrub.
v2: use fork and exec instead of system(3)

Signed-off-by: Chunyan Liu <cyliu@suse.com>

diff -r b4cf57bbc3fb tools/libxl/libxl.c
--- a/tools/libxl/libxl.cThu Oct 20 15:24:46 2011 +0800
+++ b/tools/libxl/libxl.cFri Oct 28 20:50:36 2011 +0800
@@ -1077,6 +1077,58 @@ out_free:
     libxl__free_all(&gc);
     return rc;
}
+static int fork_exec(char *arg0, char **args)
+{
+    pid_t pid;
+    int status;
+
+    pid = fork();
+    if (pid < 0)
+        return -1;
+    else if (pid == 0){
+        execvp(arg0, args);
+        exit(127);
+    }
+    sleep(1);   
+    while (waitpid(pid, &status, 0) < 0) {
+        if (errno != EINTR) {
+            status = -1;
+            break;
+        }
+    }
+
+    return status;
+}
+
+static char * nbd_mount_disk(libxl__gc *gc, libxl_device_disk *disk)
+{
+    int i;
+    int nbds_max = 16;
+    char *nbd_dev = NULL;
+    char *args[] = {"qemu-nbd","-c",NULL,NULL,NULL};
+    char *ret = NULL;
+
+    for (i = 0; i < nbds_max; i++) {
+        nbd_dev = libxl__sprintf(gc, "/dev/nbd%d", i);
+        args[2] = libxl__sprintf(gc, "%s", nbd_dev);
+        args[3] = libxl__sprintf(gc, "%s", disk->pdev_path);
+        if (fork_exec(args[0], args) == 0) {
+            ret = strdup(nbd_dev);
+            break;
+        }
+    }
+
+    return ret;
+}
+
+static int nbd_unmount_disk(libxl__gc *gc, char *diskpath) {
+    char *args[] = {"qemu-nbd","-d",NULL,NULL};
+    args[2] = libxl__sprintf(gc, "%s", diskpath);
+    if (fork_exec(args[0], args))
+        return 0;
+    else
+        return ERROR_FAIL;
+}

char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
{
@@ -1084,6 +1136,7 @@ char * libxl_device_disk_local_attach(li
     char *dev = NULL;
     char *ret = NULL;
     int rc;
+    char *mdev = NULL;

     rc = libxl__device_disk_set_backend(&gc, disk);
     if (rc) goto out;
@@ -1118,8 +1171,12 @@ char * libxl_device_disk_local_attach(li
             break;
         case LIBXL_DISK_BACKEND_QDISK:
             if (disk->format != LIBXL_DISK_FORMAT_RAW) {
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
-                           " attach a qdisk image if the format is not raw");
+                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching a non-raw qdisk image to domain 0\n");
+                mdev = nbd_mount_disk(&gc, disk);
+                if (mdev)
+                    dev = mdev;
+                else
+                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "fail to mount image with qemu-nbd");
                 break;
             }
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
@@ -1135,11 +1192,13 @@ char * libxl_device_disk_local_attach(li
  out:
     if (dev != NULL)
         ret = strdup(dev);
+    if (mdev)
+        free(mdev);
     libxl__free_all(&gc);
     return ret;
}

-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk)
+int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk, char *diskpath)
{
     /* Nothing to do for PHYSTYPE_PHY. */

@@ -1147,7 +1206,22 @@ int libxl_device_disk_local_detach(libxl
      * For other device types assume that the blktap2 process is
      * needed by the soon to be started domain and do nothing.
      */
+    libxl__gc gc = LIBXL_INIT_GC(ctx);
+    int ret;

+    switch (disk->backend) {
+        case LIBXL_DISK_BACKEND_QDISK:
+            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
+                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Locally detach a non-raw "
+                    "qdisk image");
+                ret = nbd_unmount_disk(&gc, diskpath);
+                return ret;
+            }
+        default:
+            break;
+    }
+
+    libxl__free_all(&gc);
     return 0;
}

diff -r b4cf57bbc3fb tools/libxl/libxl.h
--- a/tools/libxl/libxl.hThu Oct 20 15:24:46 2011 +0800
+++ b/tools/libxl/libxl.hFri Oct 28 20:50:36 2011 +0800
@@ -390,7 +390,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
  * Make a disk available in this domain. Returns path to a device.
  */
char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk);
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk);
+int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk, char *diskpath);

int libxl_device_nic_init(libxl_device_nic *nic, int dev_num);
int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
diff -r b4cf57bbc3fb tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.cThu Oct 20 15:24:46 2011 +0800
+++ b/tools/libxl/libxl_bootloader.cFri Oct 28 20:50:36 2011 +0800
@@ -424,7 +424,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
     rc = 0;
out_close:
     if (diskpath) {
-        libxl_device_disk_local_detach(ctx, disk);
+        libxl_device_disk_local_detach(ctx, disk, diskpath);
         free(diskpath);
     }
     if (fifo_fd > -1)

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

[-- Attachment #1.2: Type: text/html, Size: 12196 bytes --]

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

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

Re: xl create PV guest with qcow/qcow2 disk images fail

[Ian Campbell]

On Wed, 2011-11-02 at 02:58 -0400, Chun Yan Liu wrote:
> Stefano, could you review the revised patch and share your comments?
> Thanks. 
> 
> 
> >>> Chunyan Liu <cyliu@suse.com> 10/28/2011 9:27 PM >>>
> Start qemu-nbd to mount non-raw qdisk in dom0 so that xl can create PV
> guest with qcow/qcow2 disk image and using pygrub.
> v2: use fork and exec instead of system(3)
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> 
> diff -r b4cf57bbc3fb tools/libxl/libxl.c
> --- a/tools/libxl/libxl.cThu Oct 20 15:24:46 2011 +0800
> +++ b/tools/libxl/libxl.cFri Oct 28 20:50:36 2011 +0800
> @@ -1077,6 +1077,58 @@ out_free:
>      libxl__free_all(&gc);
>      return rc;
> }
> +static int fork_exec(char *arg0, char **args)
> +{
> +    pid_t pid;
> +    int status;
> +
> +    pid = fork();

This needs to be libxl_fork, I think. Perhaps even this whole thing
should be libxl__spawn_spawn (not sure about that)?

> +    if (pid < 0)
> +        return -1;
> +    else if (pid == 0){
> +        execvp(arg0, args);
> +        exit(127);
> +    }
> +    sleep(1);   

Why do you need this sleep?

> +    while (waitpid(pid, &status, 0) < 0) {
> +        if (errno != EINTR) {
> +            status = -1;
> +            break;
> +        }
> +    }
> +
> +    return status;
> +}
> +
> +static char * nbd_mount_disk(libxl__gc *gc, libxl_device_disk *disk)
> +{
> +    int i;
> +    int nbds_max = 16;
> +    char *nbd_dev = NULL;
> +    char *args[] = {"qemu-nbd","-c",NULL,NULL,NULL};

"-r" perhaps?

> +    char *ret = NULL;
> +
> +    for (i = 0; i < nbds_max; i++) {
> +        nbd_dev = libxl__sprintf(gc, "/dev/nbd%d", i);

We can't get qemu-nbd to find a free device on our behalf and tell us
what it was?

> +        args[2] = libxl__sprintf(gc, "%s", nbd_dev);
> +        args[3] = libxl__sprintf(gc, "%s", disk->pdev_path);
> +        if (fork_exec(args[0], args) == 0) {
> +            ret = strdup(nbd_dev);
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +static int nbd_unmount_disk(libxl__gc *gc, char *diskpath) {
> +    char *args[] = {"qemu-nbd","-d",NULL,NULL};
> +    args[2] = libxl__sprintf(gc, "%s", diskpath);
> +    if (fork_exec(args[0], args))
> +        return 0;
> +    else
> +        return ERROR_FAIL;
> +}
> 
> char * libxl_device_disk_local_attach(libxl_ctx *ctx,
> libxl_device_disk *disk)
> {
> @@ -1084,6 +1136,7 @@ char * libxl_device_disk_local_attach(li
>      char *dev = NULL;
>      char *ret = NULL;
>      int rc;
> +    char *mdev = NULL;
> 
>      rc = libxl__device_disk_set_backend(&gc, disk);
>      if (rc) goto out;
> @@ -1118,8 +1171,12 @@ char * libxl_device_disk_local_attach(li
>              break;
>          case LIBXL_DISK_BACKEND_QDISK:
>              if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> -                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
> -                           " attach a qdisk image if the format is
> not raw");
> +                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching a
> non-raw qdisk image to domain 0\n");
> +                mdev = nbd_mount_disk(&gc, disk);
> +                if (mdev)
> +                    dev = mdev;
> +                else
> +                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "fail to mount
> image with qemu-nbd");
>                  break;
>              }
>              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching
> qdisk %s\n",
> @@ -1135,11 +1192,13 @@ char * libxl_device_disk_local_attach(li
>   out:
>      if (dev != NULL)
>          ret = strdup(dev);
> +    if (mdev)
> +        free(mdev);

free(NULL) is acceptable.

>      libxl__free_all(&gc);
>      return ret;
> }
> 
> -int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk
> *disk)
> +int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk
> *disk, char *diskpath)
> {
>      /* Nothing to do for PHYSTYPE_PHY. */
This should be moved into the switch which you have added e.g.
	case LIBXL_DISKBACK_END_PHY:
		/* nothing to do */
		break;

> 
> @@ -1147,7 +1206,22 @@ int libxl_device_disk_local_detach(libxl
>       * For other device types assume that the blktap2 process is
>       * needed by the soon to be started domain and do nothing.

should be another explicit case statement.

>       */
> +    libxl__gc gc = LIBXL_INIT_GC(ctx);
> +    int ret;

Please declare these at the top of the function.

> 
> +    switch (disk->backend) {
> +        case LIBXL_DISK_BACKEND_QDISK:
> +            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Locally detach a
> non-raw "
> +                    "qdisk image");
> +                ret = nbd_unmount_disk(&gc, diskpath);
> +                return ret;
> +            }
> +        default:
> +            break;
> +    }
> +
> +    libxl__free_all(&gc);
>      return 0;
> }
> 
> diff -r b4cf57bbc3fb tools/libxl/libxl.h
> --- a/tools/libxl/libxl.hThu Oct 20 15:24:46 2011 +0800
> +++ b/tools/libxl/libxl.hFri Oct 28 20:50:36 2011 +0800
> @@ -390,7 +390,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
>   * Make a disk available in this domain. Returns path to a device.
>   */
> char * libxl_device_disk_local_attach(libxl_ctx *ctx,
> libxl_device_disk *disk);
> -int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk
> *disk);
> +int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk
> *disk, char *diskpath);
> 
> int libxl_device_nic_init(libxl_device_nic *nic, int dev_num);
> int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid,
> libxl_device_nic *nic);
> diff -r b4cf57bbc3fb tools/libxl/libxl_bootloader.c
> --- a/tools/libxl/libxl_bootloader.cThu Oct 20 15:24:46 2011 +0800
> +++ b/tools/libxl/libxl_bootloader.cFri Oct 28 20:50:36 2011 +0800
> @@ -424,7 +424,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>      rc = 0;
> out_close:
>      if (diskpath) {
> -        libxl_device_disk_local_detach(ctx, disk);
> +        libxl_device_disk_local_detach(ctx, disk, diskpath);
>          free(diskpath);
>      }
>      if (fifo_fd > -1)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
> 

Re: xl create PV guest with qcow/qcow2 disk images fail

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel]xl create PV guest with qcow/qcow2 disk images fail"):
> On Wed, 2011-11-02 at 02:58 -0400, Chun Yan Liu wrote:
> > +    pid = fork();
> 
> This needs to be libxl_fork, I think. Perhaps even this whole thing
> should be libxl__spawn_spawn (not sure about that)?

libxl__spawn_spawn is for processes which are going to survive past
the lifetime of the call to libxl.  I think that for bootloaders this
is going to be the case when we do event handling, so probably that's
correct.

NB that you want the version from staging as that has important
changes from Olaf regarding subprocesses.

Ian.

Re: xl create PV guest with qcow/qcow2 disk images fail

[Chunyan Liu]

[-- Attachment #1.1: Type: text/plain, Size: 7893 bytes --]

2011/11/2 Ian Campbell <Ian.Campbell@citrix.com>

> On Wed, 2011-11-02 at 02:58 -0400, Chun Yan Liu wrote:
> > Stefano, could you review the revised patch and share your comments?
> > Thanks.
> >
> >
> > >>> Chunyan Liu <cyliu@suse.com> 10/28/2011 9:27 PM >>>
> > Start qemu-nbd to mount non-raw qdisk in dom0 so that xl can create PV
> > guest with qcow/qcow2 disk image and using pygrub.
> > v2: use fork and exec instead of system(3)
> >
> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
> >
> > diff -r b4cf57bbc3fb tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.cThu Oct 20 15:24:46 2011 +0800
> > +++ b/tools/libxl/libxl.cFri Oct 28 20:50:36 2011 +0800
> > @@ -1077,6 +1077,58 @@ out_free:
> >      libxl__free_all(&gc);
> >      return rc;
> > }
> > +static int fork_exec(char *arg0, char **args)
> > +{
> > +    pid_t pid;
> > +    int status;
> > +
> > +    pid = fork();
>
> This needs to be libxl_fork, I think. Perhaps even this whole thing
> should be libxl__spawn_spawn (not sure about that)?
>

I noticed that in libxl, some places use fork() and other places use
libxl_fork(), device-model uses libxl__spawn_spwan. As for this place, I am
not clear if fork() + execvp() might cause some problem? Or it is just
considered better to use libxl_fork or libxl__spawn_spwan?


>
> > +    if (pid < 0)
> > +        return -1;
> > +    else if (pid == 0){
> > +        execvp(arg0, args);
> > +        exit(127);
> > +    }
> > +    sleep(1);
>
> Why do you need this sleep?
>

I know it seems odd. But without sleep, after executing "qemu-nbd -c" here
and passing the mount device node /dev/nbd* to fork_exec_bootloader(),
pygrub fails to parse /dev/nbd*. I am not sure if /dev/nbd* is actually not
prepared yet. But adding sleep() here or anywhere else before
fork_exec_bootloader(), then there is no problem.


> > +    while (waitpid(pid, &status, 0) < 0) {
> > +        if (errno != EINTR) {
> > +            status = -1;
> > +            break;
> > +        }
> > +    }
> > +
> > +    return status;
> > +}
> > +
> > +static char * nbd_mount_disk(libxl__gc *gc, libxl_device_disk *disk)
> > +{
> > +    int i;
> > +    int nbds_max = 16;
> > +    char *nbd_dev = NULL;
> > +    char *args[] = {"qemu-nbd","-c",NULL,NULL,NULL};
>
> "-r" perhaps?


To mount the qcow/qcow2 disk image to /dev/nbd* so that pygrub can parse
kernel and initrd from it, "-c" is enough. Of course, we can add "-r".


> > +    char *ret = NULL;
> > +
> > +    for (i = 0; i < nbds_max; i++) {
> > +        nbd_dev = libxl__sprintf(gc, "/dev/nbd%d", i);
>
> We can't get qemu-nbd to find a free device on our behalf and tell us
> what it was?
>

Well, it meant to do a thing that when "qemu-nbd -c /dev/nbd0 disk.img"
fails, it can try next nbd device. I also noticed that qemu-nbd doesn't
check if /dev/nbd* is free, even if /dev/nbd0 is already used, you can
still issue "qemu-nbd -c /dev/nbd0 disk.img". Seems no better way to check
that except "ps aux | grep /dev/nbd*". To choose a free nbd device and
mount disk, maybe write a script to do that is more proper. And at this
place, call that script.


>
> +        args[2] = libxl__sprintf(gc, "%s", nbd_dev);
> > +        args[3] = libxl__sprintf(gc, "%s", disk->pdev_path);
> > +        if (fork_exec(args[0], args) == 0) {
> > +            ret = strdup(nbd_dev);
> > +            break;
> > +        }
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int nbd_unmount_disk(libxl__gc *gc, char *diskpath) {
> > +    char *args[] = {"qemu-nbd","-d",NULL,NULL};
> > +    args[2] = libxl__sprintf(gc, "%s", diskpath);
> > +    if (fork_exec(args[0], args))
> > +        return 0;
> > +    else
> > +        return ERROR_FAIL;
> > +}
> >
> > char * libxl_device_disk_local_attach(libxl_ctx *ctx,
> > libxl_device_disk *disk)
> > {
> > @@ -1084,6 +1136,7 @@ char * libxl_device_disk_local_attach(li
> >      char *dev = NULL;
> >      char *ret = NULL;
> >      int rc;
> > +    char *mdev = NULL;
> >
> >      rc = libxl__device_disk_set_backend(&gc, disk);
> >      if (rc) goto out;
> > @@ -1118,8 +1171,12 @@ char * libxl_device_disk_local_attach(li
> >              break;
> >          case LIBXL_DISK_BACKEND_QDISK:
> >              if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> > -                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
> > -                           " attach a qdisk image if the format is
> > not raw");
> > +                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching a
> > non-raw qdisk image to domain 0\n");
> > +                mdev = nbd_mount_disk(&gc, disk);
> > +                if (mdev)
> > +                    dev = mdev;
> > +                else
> > +                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "fail to mount
> > image with qemu-nbd");
> >                  break;
> >              }
> >              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching
> > qdisk %s\n",
> > @@ -1135,11 +1192,13 @@ char * libxl_device_disk_local_attach(li
> >   out:
> >      if (dev != NULL)
> >          ret = strdup(dev);
> > +    if (mdev)
> > +        free(mdev);
>
> free(NULL) is acceptable.
>
> >      libxl__free_all(&gc);
> >      return ret;
> > }
> >
> > -int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk
> > *disk)
> > +int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk
> > *disk, char *diskpath)
> > {
> >      /* Nothing to do for PHYSTYPE_PHY. */
> This should be moved into the switch which you have added e.g.
>        case LIBXL_DISKBACK_END_PHY:
>                /* nothing to do */
>                break;
>
> >
> > @@ -1147,7 +1206,22 @@ int libxl_device_disk_local_detach(libxl
> >       * For other device types assume that the blktap2 process is
> >       * needed by the soon to be started domain and do nothing.
>
> should be another explicit case statement.
>
> >       */
> > +    libxl__gc gc = LIBXL_INIT_GC(ctx);
> > +    int ret;
>
> Please declare these at the top of the function.
>
> >
> > +    switch (disk->backend) {
> > +        case LIBXL_DISK_BACKEND_QDISK:
> > +            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> > +                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Locally detach a
> > non-raw "
> > +                    "qdisk image");
> > +                ret = nbd_unmount_disk(&gc, diskpath);
> > +                return ret;
> > +            }
> > +        default:
> > +            break;
> > +    }
> > +
> > +    libxl__free_all(&gc);
> >      return 0;
> > }
> >
> > diff -r b4cf57bbc3fb tools/libxl/libxl.h
> > --- a/tools/libxl/libxl.hThu Oct 20 15:24:46 2011 +0800
> > +++ b/tools/libxl/libxl.hFri Oct 28 20:50:36 2011 +0800
> > @@ -390,7 +390,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
> >   * Make a disk available in this domain. Returns path to a device.
> >   */
> > char * libxl_device_disk_local_attach(libxl_ctx *ctx,
> > libxl_device_disk *disk);
> > -int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk
> > *disk);
> > +int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk
> > *disk, char *diskpath);
> >
> > int libxl_device_nic_init(libxl_device_nic *nic, int dev_num);
> > int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid,
> > libxl_device_nic *nic);
> > diff -r b4cf57bbc3fb tools/libxl/libxl_bootloader.c
> > --- a/tools/libxl/libxl_bootloader.cThu Oct 20 15:24:46 2011 +0800
> > +++ b/tools/libxl/libxl_bootloader.cFri Oct 28 20:50:36 2011 +0800
> > @@ -424,7 +424,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
> >      rc = 0;
> > out_close:
> >      if (diskpath) {
> > -        libxl_device_disk_local_detach(ctx, disk);
> > +        libxl_device_disk_local_detach(ctx, disk, diskpath);
> >          free(diskpath);
> >      }
> >      if (fifo_fd > -1)
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> >
> >
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 10861 bytes --]

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

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

Re: xl create PV guest with qcow/qcow2 disk images fail

[Roger Pau Monné]

2011/11/4 Chunyan Liu <cyliu@suse.com>:
>> This needs to be libxl_fork, I think. Perhaps even this whole thing
>> should be libxl__spawn_spawn (not sure about that)?
>
> I noticed that in libxl, some places use fork() and other places use
> libxl_fork(), device-model uses libxl__spawn_spwan. As for this place, I am
> not clear if fork() + execvp() might cause some problem? Or it is just
> considered better to use libxl_fork or libxl__spawn_spwan?

If you are going to use fork to execute a file it might be best to use
vfork + libxl__exec instead. I have proposed the addition of a new
function in a patch to just do this (call vfork and libxl__exec),
which is also interesting for the execution of hotplug scripts.

Re: xl create PV guest with qcow/qcow2 disk images fail

[David Vrabel]

On 04/11/11 09:14, Roger Pau Monné wrote:
> 2011/11/4 Chunyan Liu <cyliu@suse.com>:
>>> This needs to be libxl_fork, I think. Perhaps even this whole thing
>>> should be libxl__spawn_spawn (not sure about that)?
>>
>> I noticed that in libxl, some places use fork() and other places use
>> libxl_fork(), device-model uses libxl__spawn_spwan. As for this place, I am
>> not clear if fork() + execvp() might cause some problem? Or it is just
>> considered better to use libxl_fork or libxl__spawn_spwan?

vfork() no longer exists in the POSIX.1-2008 standard so you probably
should not use it.

David

Re: xl create PV guest with qcow/qcow2 disk images fail

[Stefano Stabellini]

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

On Wed, 2 Nov 2011, Chun Yan Liu wrote:
> Stefano, could you review the revised patch and share your comments? Thanks.

Sure.
 
> >>> Chunyan Liu <cyliu@suse.com> 10/28/2011 9:27 PM >>>
> Start qemu-nbd to mount non-raw qdisk in dom0 so that xl can create PV guest with qcow/qcow2 disk image and using pygrub.
> v2: use fork and exec instead of system(3)
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> 
> diff -r b4cf57bbc3fb tools/libxl/libxl.c
> --- a/tools/libxl/libxl.cThu Oct 20 15:24:46 2011 +0800
> +++ b/tools/libxl/libxl.cFri Oct 28 20:50:36 2011 +0800
> @@ -1077,6 +1077,58 @@ out_free:
>      libxl__free_all(&gc);
>      return rc;
> }
> +static int fork_exec(char *arg0, char **args)
> +{
> +    pid_t pid;
> +    int status;
> +
> +    pid = fork();
> +    if (pid < 0)
> +        return -1;
> +    else if (pid == 0){
> +        execvp(arg0, args);
> +        exit(127);
> +    }
> +    sleep(1);

In a following email you wrote that without the sleep the device is
"not prepared yet".
What do you mean by that?
The device is present but reading/writing to it returns an error?
If so, rather than a sleep we need an explicit wait for the device
to be ready. Even trying to read from the device in a loop until it
succeeds would be better than a sleep. At least we would know exactly
what we are doing and why we are doing it.


> +    while (waitpid(pid, &status, 0) < 0) {
> +        if (errno != EINTR) {
> +            status = -1;
> +            break;
> +        }
> +    }
> +    return status;
> +}

Here you are waiting for the death of qemu-nbd; I thought that qemu-nbd
needs to be kept running?


> +static char * nbd_mount_disk(libxl__gc *gc, libxl_device_disk *disk)
> +{
> +    int i;
> +    int nbds_max = 16;
> +    char *nbd_dev = NULL;
> +    char *args[] = {"qemu-nbd","-c",NULL,NULL,NULL};
> +    char *ret = NULL;
> +
> +    for (i = 0; i < nbds_max; i++) {
> +        nbd_dev = libxl__sprintf(gc, "/dev/nbd%d", i);
> +        args[2] = libxl__sprintf(gc, "%s", nbd_dev);
> +        args[3] = libxl__sprintf(gc, "%s", disk->pdev_path);
> +        if (fork_exec(args[0], args) == 0) {
> +            ret = strdup(nbd_dev);
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}

This is not great. I would read /proc/partitions instead.
Also keep in mind that xl works on BSD now so at the very least you need
to ifdef all the linux specific code.

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

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

Re: xl create PV guest with qcow/qcow2 disk images fail

[Roger Pau Monné]

2011/11/7 Stefano Stabellini <stefano.stabellini@eu.citrix.com>:
>> +static int fork_exec(char *arg0, char **args)
>> +{
>> +    pid_t pid;
>> +    int status;
>> +
>> +    pid = fork();
>> +    if (pid < 0)
>> +        return -1;
>> +    else if (pid == 0){
>> +        execvp(arg0, args);
>> +        exit(127);
>> +    }
>> +    sleep(1);
>
> In a following email you wrote that without the sleep the device is
> "not prepared yet".
> What do you mean by that?
> The device is present but reading/writing to it returns an error?
> If so, rather than a sleep we need an explicit wait for the device
> to be ready. Even trying to read from the device in a loop until it
> succeeds would be better than a sleep. At least we would know exactly
> what we are doing and why we are doing it.

I really don't understand why a sleep is needed before executing
waitpid, I would understand that you wait before accessing the device
(although, as Stefano noticed, it's not the preferred way), but this
wait should not be done here, in fact I think the fork_exec function
should be added to libxl_exec.c and made public. I have a patch with a
libxl__forkexec function prepared for submission, which might come in
handy here.

>> +    while (waitpid(pid, &status, 0) < 0) {
>> +        if (errno != EINTR) {
>> +            status = -1;
>> +            break;
>> +        }
>> +    }
>> +    return status;
>> +}
>> +static char * nbd_mount_disk(libxl__gc *gc, libxl_device_disk *disk)
>> +{
>> +    int i;
>> +    int nbds_max = 16;
>> +    char *nbd_dev = NULL;
>> +    char *args[] = {"qemu-nbd","-c",NULL,NULL,NULL};
>> +    char *ret = NULL;
>> +
>> +    for (i = 0; i < nbds_max; i++) {
>> +        nbd_dev = libxl__sprintf(gc, "/dev/nbd%d", i);
>> +        args[2] = libxl__sprintf(gc, "%s", nbd_dev);
>> +        args[3] = libxl__sprintf(gc, "%s", disk->pdev_path);
>> +        if (fork_exec(args[0], args) == 0) {
>> +            ret = strdup(nbd_dev);
>> +            break;
>> +        }
>> +    }
>> +
>> +    return ret;
>> +}
>
> This is not great. I would read /proc/partitions instead.
> Also keep in mind that xl works on BSD now so at the very least you need
> to ifdef all the linux specific code.

I would rather say that xl is closer to working on BSD now, but nbd is
not supported on BSD (neither is qdisk), so anything related to nbd
should be keept as Linux specific code.

Regards, Roger.

Re: xl create PV guest with qcow/qcow2 disk images fail

[Chunyan Liu]

[-- Attachment #1.1: Type: text/plain, Size: 3372 bytes --]

2011/11/7 Stefano Stabellini <stefano.stabellini@eu.citrix.com>

> On Wed, 2 Nov 2011, Chun Yan Liu wrote:
> > Stefano, could you review the revised patch and share your comments?
> Thanks.
>
> Sure.
>
> > >>> Chunyan Liu <cyliu@suse.com> 10/28/2011 9:27 PM >>>
> > Start qemu-nbd to mount non-raw qdisk in dom0 so that xl can create PV
> guest with qcow/qcow2 disk image and using pygrub.
> > v2: use fork and exec instead of system(3)
> >
> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
> >
> > diff -r b4cf57bbc3fb tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.cThu Oct 20 15:24:46 2011 +0800
> > +++ b/tools/libxl/libxl.cFri Oct 28 20:50:36 2011 +0800
> > @@ -1077,6 +1077,58 @@ out_free:
> >      libxl__free_all(&gc);
> >      return rc;
> > }
> > +static int fork_exec(char *arg0, char **args)
> > +{
> > +    pid_t pid;
> > +    int status;
> > +
> > +    pid = fork();
> > +    if (pid < 0)
> > +        return -1;
> > +    else if (pid == 0){
> > +        execvp(arg0, args);
> > +        exit(127);
> > +    }
> > +    sleep(1);
>
> In a following email you wrote that without the sleep the device is
> "not prepared yet".
> What do you mean by that?
> The device is present but reading/writing to it returns an error?
>
If so, rather than a sleep we need an explicit wait for the device
> to be ready. Even trying to read from the device in a loop until it
> succeeds would be better than a sleep. At least we would know exactly
> what we are doing and why we are doing it.
>

OK, after checking qemu-nbd source code, I think I know where the problem
is. I tried to fork_exec "qemu-nbd -c /dev/nbd0 disk.img", with this
command option, qemu-nbd will call daemon(3) to run in background and
itself should exit immediately, that's why waitpid can successfully wait
the exit of qemu-nbd pid and fork_exec will return 0. The problem is that I
should not use fork_exec return value to decide if the disk.img is already
connected to nbd device. That's not correct.


> > +    while (waitpid(pid, &status, 0) < 0) {
> > +        if (errno != EINTR) {
> > +            status = -1;
> > +            break;
> > +        }
> > +    }
> > +    return status;
> > +}
>
> Here you are waiting for the death of qemu-nbd; I thought that qemu-nbd
> needs to be kept running?
>
> Same as above.


> > +static char * nbd_mount_disk(libxl__gc *gc, libxl_device_disk *disk)
> > +{
> > +    int i;
> > +    int nbds_max = 16;
> > +    char *nbd_dev = NULL;
> > +    char *args[] = {"qemu-nbd","-c",NULL,NULL,NULL};
> > +    char *ret = NULL;
> > +
> > +    for (i = 0; i < nbds_max; i++) {
> > +        nbd_dev = libxl__sprintf(gc, "/dev/nbd%d", i);
> > +        args[2] = libxl__sprintf(gc, "%s", nbd_dev);
> > +        args[3] = libxl__sprintf(gc, "%s", disk->pdev_path);
> > +        if (fork_exec(args[0], args) == 0) {
> > +            ret = strdup(nbd_dev);
> > +            break;
> > +        }
> > +    }
> > +
> > +    return ret;
> > +}
>
> This is not great. I would read /proc/partitions instead.
>
Thanks, that's a better way to find if a nbd device is free. The whole
thing (find a free nbd device and connect disk.img to that nbd device)
seems better to write a script to do that and in libxl call that script.


> Also keep in mind that xl works on BSD now so at the very least you need
> to ifdef all the linux specific code.
>

My fault, thanks for reminding.

[-- Attachment #1.2: Type: text/html, Size: 4991 bytes --]

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

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

Re: xl create PV guest with qcow/qcow2 disk images fail

[Stefano Stabellini]

On Wed, 9 Nov 2011, Chunyan Liu wrote:
> In a following email you wrote that without the sleep the device is
> "not prepared yet".
> What do you mean by that?
> The device is present but reading/writing to it returns an error?
> 
>       If so, rather than a sleep we need an explicit wait for the device
>       to be ready. Even trying to read from the device in a loop until it
>       succeeds would be better than a sleep. At least we would know exactly
>       what we are doing and why we are doing it.
> 
> 
> OK, after checking qemu-nbd source code, I think I know where the problem is. I tried to fork_exec "qemu-nbd -c /dev/nbd0
> disk.img", with this command option, qemu-nbd will call daemon(3) to run in background and itself should exit immediately,
> that's why waitpid can successfully wait the exit of qemu-nbd pid and fork_exec will return 0. The problem is that I
> should not use fork_exec return value to decide if the disk.img is already connected to nbd device. That's not correct.

Good.


> This is not great. I would read /proc/partitions instead.
> 
> Thanks, that's a better way to find if a nbd device is free. The whole thing (find a free nbd device and connect disk.img
> to that nbd device) seems better to write a script to do that and in libxl call that script.

Maybe, but I think it makes sense for it to be in libxl and after all
reading/writing a file can be done quite well in a library too.

Re: xl create PV guest with qcow/qcow2 disk images fail

[Ian Campbell]

On Wed, 2011-11-09 at 09:27 +0000, Stefano Stabellini wrote:
> On Wed, 9 Nov 2011, Chunyan Liu wrote:
> > In a following email you wrote that without the sleep the device is
> > "not prepared yet".
> > What do you mean by that?
> > The device is present but reading/writing to it returns an error?
> > 
> >       If so, rather than a sleep we need an explicit wait for the device
> >       to be ready. Even trying to read from the device in a loop until it
> >       succeeds would be better than a sleep. At least we would know exactly
> >       what we are doing and why we are doing it.
> > 
> > 
> > OK, after checking qemu-nbd source code, I think I know where the problem is. I tried to fork_exec "qemu-nbd -c /dev/nbd0
> > disk.img", with this command option, qemu-nbd will call daemon(3) to run in background and itself should exit immediately,
> > that's why waitpid can successfully wait the exit of qemu-nbd pid and fork_exec will return 0. The problem is that I
> > should not use fork_exec return value to decide if the disk.img is already connected to nbd device. That's not correct.
> 
> Good.
> 
> 
> > This is not great. I would read /proc/partitions instead.
> > 
> > Thanks, that's a better way to find if a nbd device is free. The whole thing (find a free nbd device and connect disk.img
> > to that nbd device) seems better to write a script to do that and in libxl call that script.

Really this is the sort of thing qemu-nbd should do for us (c.f.
"losetup -f"). I think it would be worth getting such a patch upstream
even if we can't yet make use of it.

Ian.

> 
> Maybe, but I think it makes sense for it to be in libxl and after all
> reading/writing a file can be done quite well in a library too.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel