maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

andrei.elkin@xxxxxxxxxx writes:

> Sujatha, Kristian, howdy.

Hi Andrei!

> Kristian, the question was about `xid_count_per_binlog' struct
> comment -- ..
>
>
>   struct xid_count_per_binlog : public ilink {
>     char *binlog_name;
>     uint binlog_name_len;
>     ulong binlog_id;
>     /* Total prepared XIDs and pending checkpoint requests in this binlog. */
>     long xid_count;
>     /* For linking in requests to the binlog background thread. */
>     xid_count_per_binlog *next_in_queue;
>
>    xid_count_per_binlog();   /* Give link error if constructor used. */
>
> .. -----------------------------^
>  };
>
> Now I am guessing a constructor was attempted to fail out.. Was that
> related to de-POD-ing of the object ('cos of the constructor)? It's
> pretty obscure.
> We would appreciate your explanation.

The intention was (IIRC, this is 7 year old code!) to use just POD, no need
to employ virtual destructors and vtables and so on for this.

But yes, given that struct ilink has a virtual destructor, that's not a
valid use :-/

So keeping with the original intent of the code, the fix would be different:
to replace the struct ilink and convert the data structure into a real POD.

But I don't think it's that important one way or the other, this patch is
probably fine as well (the allocation of this struct is only once per binlog
rotation, so performance impact is probably of low concern).

The main concern is that with this patch, the ~ilink() destructor is called,
which will corrupt the list if any dangling prev/next pointers are left in
the object being destroyed. But from my reading of the code, all calls of
the destructor looks safe in the patch.

>> -    xid_count_per_binlog();   /* Give link error if constructor used. */
>
> Maybe we should leave it as it seems to be for catching inadvertent
> object constructions.
> I'm cc-ing Kristian to clear out.

Well, the patch changes the code to explicitly use the contructor. So this
line is no longer relevant, and removing is correct.

So in summary, I think generally it is best to keep to POD for things like
this. But the patch seems an easy way to avoid the illegal mix of POD with
virtual destructor, so is probably ok,

 - Kristian.