Re: [Xen-devel]xl create PV guest with qcow/qcow2 disk images fail

[Chunyan Liu <cyliu@xxxxxxxx>]

2011/11/2 Ian Campbell <Ian.Campbell@xxxxxxxxxx>

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@xxxxxxxx> 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@xxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
>
>

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