[libvirt] [GSoC] Code design for scalar and external types

[Sukrit Bhatnagar]

On Mon, 18 Jun 2018 at 17:20, Andrea Bolognani <abologna at redhat.com> wrote:
>
> On Mon, 2018-06-18 at 11:52 +0100, Daniel P. Berrangé wrote:
> > On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> > > # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > >     static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > >     { \
> > >         (func)(_ptr); \
> > >     }
> >
> > For anything where we define the impl of (func) these two macros
> > should never be used IMHO. We should just change the signature of
> > the real functions so that accept a double pointer straight away,
> > and thus not require the wrapper function. Yes, it this will
> > require updating existing code, but such a design change is
> > beneficial even without the cleanup macros, as it prevents the
> > possbility of double frees. I'd suggest we just do this kind of
> > change straightaway
>
> Agreed that we want to fix our own free functions.
>
> > > # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> >
> > I don't see the point in passing "type" in here we could simplify
> > this:
> >
> >   #define VIR_AUTOFREE __attribute__((cleanup(virFree))
> >
> >   VIR_AUTOFREE char *foo = NULL;
>
> Passing the type was suggested earlier to make usage consistent
> across all cases, eg.
>
>   VIR_AUTOFREE(char *) str = NULL;
>   VIR_AUTOPTR(virDomain) dom = NULL;
>
> and IIRC we ended up agreeing that it was a good idea overall.
>
> > > # define VIR_AUTOPTR(type) \
> > >     __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) VIR_AUTOPTR_TYPE_NAME(type)
> > >
> > >       The usage would not require our internal vir*Ptr types and would
> > >       be easy to use with external types as well:
> > >
> > > VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
> > >
> > > VIR_AUTOPTR(virBitmap) map = NULL;
> > >
> > > VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
> >
> > Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
> > this is a reasonable usage, because we don't control the signature
> > of the libssh2_channel_free function.
>
> This is why I suggested we might want to have two sets of
> macros, one for types we defined ourselves and one for types
> we bring in from external libraries.
>
> > > VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
> >
> > With my example above
> >
> >    #define VIR_AUTOFREE_LIBSSH_CHANNEL VIR_AUTOFREE_FUNC(LIBSSH2_CHANNEL)
> >
> >    VIR_AUTOFREE_LIBSSH_CHANNEL LIBSSH_CHANNEL *chan = NULL;
>
> This makes the declaration needlessly verbose, since you're
> repeating the type name twice; Pavel's approach would avoid
> that.

So, have we reached a decision about the design?

Fixing the existing vir*Free functions to take double pointers seems good to me,
as many have mentioned earlier. It will solve multiple problems at once.
I have almost 7 weeks left of GSoC, and I think it is totally doable.

On the other hand, making cleanup macros use double pointers also seems
great! But, aren't we borrowing too much design from GLib?

Anyway, both approaches take care of the double free problem.

TL;DR
I prefer cleanup macros which define functions with double pointers
over changing
existing vir*Free functions. It just looks more natural.

I apologize in advance for this long mail! Could not decide what
patches to make,
so I included a glimpse of the code in this mail.
This is just to start this discussion again and help with visualizing.

Here goes some example code. Most of it is paraphrasing what is already
discussed previously.

/******************************************************************************
* Cleanup macros defining functions with double pointers:
******************************************************************************/
#define VIR_AUTOPTR_TYPE_NAME(type) type##AutoPtr
#define VIR_AUTOPTR_FUNC_NAME(type) \
    VIR_AUTOPTR_TYPE_NAME(type)##Free
#define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
    typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
    static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
    { \
        if (*_ptr) \
            (func)(*_ptr); \
        *_ptr = NULL; \
    } \
#define VIR_AUTOPTR(type) \
        __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) \
                VIR_AUTOPTR_TYPE_NAME(type)


///////// Using this for virBitmap:

VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree)
will generate:

typedef virBitmap *virBitmapAutoPtr;
static inline void virBitmapAutoPtrFree(virBitmap **_ptr)
{
    if (*_ptr)
        virBitmapFree(*_ptr);
    *_ptr = NULL;
}

Then we use it like:
VIR_AUTOPTR(virBitmap) map = NULL;

which will become:
__attribute__((cleanup(virBitmapAutoPtrFree))) virBitmapAutoPtr map = NULL;


///////// Using this for char ** (some of my ideas added):

typedef char *virString;

VIR_DEFINE_AUTOPTR_FUNC(virString, virStringListFree)
will generate:

typedef virString *virStringAutoPtr;
static inline void virStringAutoPtrFree(virString **_ptr)
{
    if (*_ptr)
        virStringListFree(*_ptr);
    *_ptr = NULL;
}

Then we use it like:
VIR_AUTOPTR(virString) lines = NULL;

which will become:
__attribute__((cleanup(virStringAutoPtrFree))) virStringAutoPtr lines = NULL;


///////// Using this for External types:

VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free)
will generate:

typedef LIBSSH2_CHANNEL *LIBSSH2_CHANNELAutoPtr;
static inline void LIBSSH2_CHANNELAutoPtrFree(LIBSSH2_CHANNEL **_ptr)
{
    if (*_ptr)
        libssh2_channel_free(*_ptr);
    *_ptr = NULL;
}

Then we use it like:
VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;

which will become:
__attribute__((cleanup(LIBSSH2_CHANNELAutoPtrFree)))
                                LIBSSH2_CHANNELAutoPtr channel = NULL;


/******************************************************************************
* Changing the existing vir*Free functions to accept double pointers:
* (not including the changes made in the above one, but reusing our
* original design)
******************************************************************************/
#define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
#define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
    static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
    { \
        (func)(_ptr); \
    } \
#define VIR_AUTOPTR(type) \
    __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type


///////// Using this for virBitmap:

void
virBitmapFree(virBitmapPtr bitmap)
{
    if (bitmap) {
        VIR_FREE(bitmap->map);
        VIR_FREE(bitmap);
    }
}

will look like this:

void
virBitmapFree(virBitmapPtr *bitmap)
{
    if (*bitmap) {
        VIR_FREE((*bitmap)->map);
        VIR_FREE(*bitmap);
    }
    *bitmap = NULL;
}

VIR_DEFINE_AUTOPTR_FUNC(virBitmapPtr, virBitmapFree)
will generate:

static inline void virAutoPtrvirBitmapPtr(virBitmapPtr *_ptr)
{
    virBitmapFree(_ptr);
}

Then we use it like:
VIR_AUTOPTR(virBitmapPtr) map = NULL;

which will become:
__attribute__((cleanup(virBitmapPtrAutoPtrFree))) virBitmapPtr map = NULL;

But, then each (explicit) occurrence of this:
virBitmapFree(bitmap);

will have to be like this:
virBitmapFree(&bitmap);


///////// Using this for char **:

void virStringListFree(char **strings)
{
    char **tmp = strings;
    while (tmp && *tmp) {
        VIR_FREE(*tmp);
        tmp++;
    }
    VIR_FREE(strings);
}

will look like this:

void virStringListFree(char ***strings)
OR
void virStringListFree(virStringList *strings)
{
    char ***tmp = strings;
    OR
    virStringList *tmp = strings;

    while (*tmp && **tmp) {
        VIR_FREE(**tmp);
        *tmp++;
    }
    VIR_FREE(*strings);
    *strings = NULL;
}

typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
will generate:

static inline void virStringListAutoPtrFree(virStringList *_ptr)
{
    virStringListFree(_ptr);
}

Then we use it like:
VIR_AUTOPTR(virStringList) lines = NULL;

which will become:
__attribute__((cleanup(virStringListAutoPtrFree))) virStringList lines = NULL;

But again, this:
virStringListFree(lines);

will have to be like this:
virStringListFree(&lines);


///////// For External types?:
We may need to make separate macros for External types. If so, we can
reuse the macros in the original design for them. Something like:

# define VIR_DEFINE_EXT_AUTOPTR_FUNC(type, func) \
    static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
    { \
        (func)(*_ptr); \
    } \
# define VIR_EXT_AUTOPTR(type) \
    __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type

Something like this might have to be done for all External types:
typedef LIBSSH2_CHANNEL *LIBSSH2_CHANNEL_PTR;

VIR_DEFINE_EXT_AUTOPTR_FUNC(LIBSSH2_CHANNEL_PTR, libssh2_channel_free);
will generate:

static inline void virAutoPtrLIBSSH2_CHANNEL_PTR(LIBSSH2_CHANNEL_PTR *_ptr)
{
        libssh2_channel_free(*_ptr);
}

Then we use it like:
VIR_EXT_AUTOPTR(LIBSSH2_CHANNEL_PTR) channel = NULL;

which will become:
__attribute__((cleanup(virAutoPtrLIBSSH2_CHANNEL)))
                            LIBSSH2_CHANNEL_PTR channel = NULL;


Thanks,
Sukrit