Libvirt Open Source Contribution

[Barrett J Schonefeld]

Hey folks,

We appreciate the feedback, and we've used this to complete some initial
work on issue 11.

We started with small changes in src/util, and we submitted them via email
(following these guidelines on submitting patches
<https://libvirt.org/submitting-patches.html>) to ensure we are on the
right path.

We do not see the patch we submitted via email showing up in the threads,
and we are wondering if we are submitting incorrectly or if the patch goes
through some approval process before appearing in the threads.

Best regards,
Barrett Schonefeld

On Tue, Nov 3, 2020 at 4:02 AM Michal Privoznik <mprivozn at redhat.com> wrote:

> On 11/3/20 12:47 AM, Barrett J Schonefeld wrote:
> > Hey folks,
> >
> > We have started work on issue 11
> > <https://gitlab.com/libvirt/libvirt/-/issues/11>, and we have some
> > questions to ensure we tackle the issue properly.
> >
> >
> >     - What are the different use cases for g_autoptr vs g_autofree? We
> found
> >     that g_autofree is preferred for anything that uses g_malloc
> according to
> >     the Glib documentation
> >     <
> https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree
> >,
> >     and g_autoptr is for types with custom destructors. However, when
> using
> >     g_autoptr, we got compile errors when trying to pass the g_autoptr
> as an
> >     argument (the argument seems to be converted to an integer). When
> should we
> >     use each of these, and when should we not convert them at all?
>
> Right. g_autofree instructs compiler to call plain free() over the
> variable when it goes out of scope (mind you, not only at the end of a
> function, but also inside a loop, if-else bodies, etc.) while
> g_autoptr() does two things: it declares variable as a pointer to given
> type, and calls previously registered destructor when the variable goes
> out of scope (instead of plain free()). For instance:
>
>    g_autofree char *tmp = g_strdup("my awesome string");
>    g_autoptr(virBitmap) b = virBitmapNew(..);
>
> And since virBitmap module lives under src/util/virbitmap.* that's also
> where you can find the destructor registration:
>
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virBitmap, virBitmapFree);
>
> We are currently in transition state of adopting glib so you can find a
> mixture of our old approach (VIR_FREE() and/or explicit destructor calls
> like virBitmapFree()) and the new approach (where glib is used). Not
> ideal, but for any new code we try to use glib if possible. Sometimes we
> just rewrite a module/file into glib, for instance:
>
> https://www.redhat.com/archives/libvir-list/2020-October/msg00210.html
>
> Hence, any rewrite to glib is welcome!
>
> >
> >     - We see that some work has been done to convert files to use the
> Glib
> >     API. In some cases, files contain code that uses both the old memory
> >     management API and the Glib API. Should we focus our attention on
> files
> >     where these conversions are not yet underway? Or should we expect
> that many
> >     of the files are only partially converted?
>
> You are free to chose whatever you want. I'd start with small files
> (e.g. src/util/*) because they are usually self contained and have small
> functions in them (i.e. no complicated branching is happening inside a
> function).
>
> >
> >     - In some cases, we found that converting to the Glib API might
> require
> >     cascading changes to code structure (function parameter types,
> function
> >     return types). Is it advisable to pursue these cases as well, or
> should we
> >     limit changes to pointers which are declared and then freed within
> one
> >     method?
>
> This is a tougher question. Libvirt has a promise of stable API which
> includes public C API and ABI. Therefore an user facing function can't
> change its parameters or their order. These are declared in
> include/libvirt/*.h and implemented in src/libvirt-. Note, the body of
> these functions can be changed, it's signature that can never change.
> Having said that, our internal APIs are free to change as we please.
>
> >
> >     - Do you know of a directory or set of files where the conversions to
> >     the Glib API are needed?
>
> IMO, it's best to start with something smaller (like src/util/, tools/)
> send that for review to make sure you're on the right path and then
> continue with something bigger.
>
>
> Michal
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201105/44e78dd1/attachment-0001.htm>