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

[Roger Pau Monné <roger.pau@xxxxxxxxxxxxx>]

2011/11/7 Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>:
>> +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.

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