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

[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.


> +Â Â Â  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.

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