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

[Chunyan Liu <cyliu@xxxxxxxx>]

2011/11/7 Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

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@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();
> +    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.

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