[Libguestfs] Libguestfs gobject bindings

[Richard W.M. Jones]

On Fri, Jan 13, 2012 at 11:40:40AM +0000, Daniel P. Berrange wrote:
> On Fri, Jan 13, 2012 at 11:33:33AM +0000, Matthew Booth wrote:
> > On 01/13/2012 10:58 AM, Daniel P. Berrange wrote:
> > >On Thu, Jan 12, 2012 at 11:59:22AM +0000, Matthew Booth wrote:
> > >>I'm currently working on gobject bindings for libguestfs. I haven't
> > >>got as far as compiling anything yet, but I've attached the C header
> > >>for initial review.
> > >>typedef struct _GuestfsPV GuestfsPV;
> > >>struct _GuestfsPV {
> > >>   gchar *pv_name;
> > >>   /* The next field is NOT nul-terminated, be careful when printing it: */
> > >>   gchar pv_uuid[32];
> > >
> > >How about a:
> > >
> > >    #define GUESTFS_LVM_UUID_BUFLEN 32
> > >
> > >and use that throughout
> > 
> > I don't think this one is massively important, because you can
> > always use sizeof pv_uuid as an equivalent constant. As the API is
> > automatically generated, these are always going to be consistent.

I think keep the definition in the gobject header.

> sizeof() works fine for C, but not for languages. If you add
> the constant, then it'll get exposed to the languages automatically.
>
> > >For the vast majority of these, or at least, any which take non-trivial
> > >execution time, I think it would be important to add _async variants,
> > >following the GIO design pattern. eg
> > >
> > >   void guest_session_tune2fs_async(GuestfsSession *session, const gchar *device,
> > >                                    GuestfsTune2FS *optargs,
> > >                                    GCancellable *cancellable,
> > >                                    GAsyncReadyCallback callback,
> > >                                    gpointer user_data);
> > >   gboolean guest_session_tune2fs_finish(GAsyncResult *result,
> > >                                         GError **error);
> > >
> > >This is a style that is widely preferred by people writing modern GTK
> > >applications, since it avoids the need to use threads in GUI apps.

Tricky to actually implement this though, without either creating a
hidden thread somewhere, or substantially rearchitecting the
libguestfs C API.

virt-manager & guestfs-browser use libguestfs by creating a separate
thread and sending it instructions.  It's not that hard to implement,
so for the moment I'd say forget about providing async functions.

> > Sounds like a good idea. Can I assume that I can safely add this at
> > a later date without taking it into account at this stage? If so,
> > I'll come back to this once I have a working binding.
> 
> Yes, it is easy to add in later.
> 
> The only issue would be if you wanted to add in 'GCancellable' to
> the existing blocking APIs. I imagine there's no easy way to allow
> cancellation of API calls at the libguestfs level, so it is probably
> not worth it.

Actually, this is possible for some (by no means all) calls:
http://libguestfs.org/guestfs.3.html#cancelling_long_transfers
You can also kill the qemu subprocess, although you risk losing any
state you were writing to the disk.

> > Incidentally, I'm also playing with changes to functions with return
> > or take array. I've decided to drop GSList in favour of a
> > NULL-terminated array as returned by guestfs. For BufferIn and
> > RBufferOut I've dropped the GByteArray in favour of a guint8 * with
> > an additional length parameter.
> 
> Hmm, it is probably better to use GInputStream & GOutputStream for
> any bulk I/O APIs. That way apps have the flexibility to give
> you a GFileInputStream, or a GMemoryInputStream as they see fit,
> rather than forcing them to have it all in memory.

First of all, BufferIn and RBufferOut are "small" buffers, always less
than 4MB.  RBufferOut is already in memory, so feeding it into a
stream is pointless.

The issue is more likely to apply to FileIn and FileOut.  These won't
map very easily to GInputStream/GOutputStream, at least not without a
hidden thread.

BTW all of the above is handled better in wrappi :-)

> > My concern is around memory handling in the bindings. An RStringList
> > function returns a NULL-terminated char **. If I cast that to gchar
> > ** and return it as-is, will bindings correctly garbage collect it?
> 
> Every parameter and return value as a 'transfer' annotation which
> controls memory management. For objects it is either 'none' (receiver
> of object must not free it), or 'full' (receiver of object must free
> it).
> 
> For lists/hashes there is extra values, to cope with the scenario
> where the receiver must free the List/Hash, but not the values
> stored in it. See:
> 
>   https://live.gnome.org/GObjectIntrospection/Annotations

The caller must free the strings and the array.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v