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