Re: [Xen-devel] [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 29 May 2012, Stefano Stabellini wrote:
> On Tue, 29 May 2012, Ian Campbell wrote:
> > On Tue, 2012-05-29 at 11:39 +0100, Stefano Stabellini wrote:
> > > Introduce a new libxl_device_disk* parameter to
> > > libxl__device_disk_local_attach, the parameter is allocated by the
> > > caller. libxl__device_disk_local_attach is going to fill the new disk
> > > with informations about the new locally attached disk.  The new
> > > libxl_device_disk should be passed to libxl_device_disk_local_detach
> > > afterwards.
> > > 
> > > Changes in v8:
> > > - free pdev_path and script in local_detach;
> > > - use libxl__strdup instead of strdup.
> > > 
> > > Changes in v7:
> > > - rename tmpdisk to localdisk;
> > > - add a comment in libxl__bootloader_state about localdisk.
> > > 
> > > Changes in v6:
> > > - return error in case pdev_path is NULL;
> > > - libxl__device_disk_local_attach update the new disk, the caller
> > > allocates it;
> > > - remove \n from logs.
> > > 
> > > Changes in v5:
> > > - rename disk to in_disk;
> > > - rename tmpdisk to disk;
> > > - copy disk to new_disk only on success;
> > > - remove check on libxl__zalloc success.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > ---
> > >  tools/libxl/libxl.c            |   18 +++++++++++++++---
> > >  tools/libxl/libxl_bootloader.c |    4 ++--
> > >  tools/libxl/libxl_internal.h   |   10 +++++++++-
> > >  3 files changed, 26 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index cd870c4..762e72a 100644
> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -1735,13 +1735,24 @@ out:
> > >      return ret;
> > >  }
> > >  
> > > -char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk 
> > > *disk)
> > > +char * libxl__device_disk_local_attach(libxl__gc *gc,
> > > +        const libxl_device_disk *in_disk,
> > > +        libxl_device_disk *disk)
> > 
> > Why the weird indent?
> 
> Uhm.. I don't know, I think it was just the default on vim.
> 
> 
> > >  {
> > >      libxl_ctx *ctx = gc->owner;
> > >      char *dev = NULL;
> > >      char *ret = NULL;
> > >      int rc;
> > >  
> > > +    if (in_disk->pdev_path == NULL)
> > > +        return NULL;
> > > +
> > > +    memcpy(disk, in_disk, sizeof(libxl_device_disk));
> > > +    disk->pdev_path = libxl__strdup(NULL, in_disk->pdev_path);
> > > +    if (in_disk->script != NULL)
> > > +        disk->script = libxl__strdup(NULL, in_disk->script);
> > > +    disk->vdev = NULL;
> > > +
> > >      rc = libxl__device_disk_setdefault(gc, disk);
> > >      if (rc) goto out;
> > >  
> > > @@ -1779,8 +1790,7 @@ char * libxl__device_disk_local_attach(libxl__gc 
> > > *gc, libxl_device_disk *disk)
> > >                             " attach a qdisk image if the format is not 
> > > raw");
> > >                  break;
> > >              }
> > > -            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk 
> > > %s\n",
> > > -                       disk->pdev_path);
> > > +            LOG(DEBUG, "locally attaching qdisk %s", in_disk->pdev_path);
> > >              dev = disk->pdev_path;
> > >              break;
> > >          default:
> > > @@ -1804,6 +1814,8 @@ int libxl__device_disk_local_detach(libxl__gc *gc, 
> > > libxl_device_disk *disk)
> > >       * needed by the soon to be started domain and do nothing.
> > >       */
> > >  
> > > +    free(disk->pdev_path);
> > > +    free(disk->script);
> > 
> > This is open coding libxl_device_disk_dispose(disk) but missed the vdev
> > member, is that deliberate?
> 
> I think it is a mistake: all these strings used to be allocated on the
> gc, on previous versions of the series. However meanwhile the event
> series went in, changing completely libxl__bootloader_run (that is the
> caller of libxl__device_disk_local_attach).
> Allocating stuff on the gc is not correct anymore, so now they need to
> be explicitly freed. I think I should call libxl_device_disk_dispose
> here and strdup in libxl__device_disk_local_attach to make sure vdev is
> not on the gc.

The appended patch switches back the behavior to what we had in v5 and
before: pdev_path, script, and vdev are all allocated on the gc,
therefore freed automatically.


---

libxl: libxl__device_disk_local_attach return a new libxl_device_disk

Introduce a new libxl_device_disk* parameter to
libxl__device_disk_local_attach, the parameter is allocated by the
caller. libxl__device_disk_local_attach is going to fill the new disk
with informations about the new locally attached disk.  The new
libxl_device_disk should be passed to libxl_device_disk_local_detach
afterwards.

Changes in v9:
- allocate pdev_path, script, and vdev on the gc.

Changes in v8:
- free pdev_path and script in local_detach;
- use libxl__strdup instead of strdup.

Changes in v7:
- rename tmpdisk to localdisk;
- add a comment in libxl__bootloader_state about localdisk.

Changes in v6:
- return error in case pdev_path is NULL;
- libxl__device_disk_local_attach update the new disk, the caller
allocates it;
- remove \n from logs.

Changes in v5:
- rename disk to in_disk;
- rename tmpdisk to disk;
- copy disk to new_disk only on success;
- remove check on libxl__zalloc success.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index cd870c4..be1c900 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1735,13 +1735,24 @@ out:
     return ret;
 }
 
-char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
+char * libxl__device_disk_local_attach(libxl__gc *gc,
+        const libxl_device_disk *in_disk,
+        libxl_device_disk *disk)
 {
     libxl_ctx *ctx = gc->owner;
     char *dev = NULL;
     char *ret = NULL;
     int rc;
 
+    if (in_disk->pdev_path == NULL)
+        return NULL;
+
+    memcpy(disk, in_disk, sizeof(libxl_device_disk));
+    disk->pdev_path = libxl__strdup(gc, in_disk->pdev_path);
+    if (in_disk->script != NULL)
+        disk->script = libxl__strdup(gc, in_disk->script);
+    disk->vdev = NULL;
+
     rc = libxl__device_disk_setdefault(gc, disk);
     if (rc) goto out;
 
@@ -1779,8 +1790,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, 
libxl_device_disk *disk)
                            " attach a qdisk image if the format is not raw");
                 break;
             }
-            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
-                       disk->pdev_path);
+            LOG(DEBUG, "locally attaching qdisk %s", in_disk->pdev_path);
             dev = disk->pdev_path;
             break;
         default:
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index e8950b1..82371f1 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -228,7 +228,7 @@ static void bootloader_cleanup(libxl__egc *egc, 
libxl__bootloader_state *bl)
     if (bl->outputdir) libxl__remove_directory(gc, bl->outputdir);
 
     if (bl->diskpath) {
-        libxl__device_disk_local_detach(gc, bl->disk);
+        libxl__device_disk_local_detach(gc, &bl->localdisk);
         free(bl->diskpath);
         bl->diskpath = 0;
     }
@@ -330,7 +330,7 @@ void libxl__bootloader_run(libxl__egc *egc, 
libxl__bootloader_state *bl)
         goto out;
     }
 
-    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk);
+    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk, 
&bl->localdisk);
     if (!bl->diskpath) {
         rc = ERROR_FAIL;
         goto out;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d34e561..21b8b54 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1265,7 +1265,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, 
char *be_path);
  * a device.
  */
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
-        libxl_device_disk *disk);
+        const libxl_device_disk *in_disk,
+        libxl_device_disk *new_disk);
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
         libxl_device_disk *disk);
 
@@ -1803,6 +1804,13 @@ struct libxl__bootloader_state {
     libxl__bootloader_console_callback *console_available;
     libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated */
     libxl_device_disk *disk;
+    /* Should be zeroed by caller on entry.  Will be filled in by
+     * bootloader machinery; represents the local attachment of the
+     * disk for the benefit of the bootloader.  Must be detached by
+     * the caller using libxl__device_disk_local_detach, but only
+     * after the domain's kernel and initramfs have been loaded into
+     * memory and the file references disposed of. */
+    libxl_device_disk localdisk;
     uint32_t domid;
     /* private to libxl__run_bootloader */
     char *outputpath, *outputdir, *logfile;

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel