[PATCH] Fix wrong talloc context, remove queue_destructor and minor updates.

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix wrong talloc context, remove queue_destructor and minor updates.

Samba - samba-technical mailing list
Hi

This is my first patch to SAMBA.
I'm quiet sure it's easy stuff but I'm trying to gently introduce
myself to the community.
Please go easy on me if I failed already some basics at the beginning.

Please review and comment.
Thanks in advance.

Cheers Swen

ctdb_io_queue_setup.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix wrong talloc context, remove queue_destructor and minor updates.

Samba - samba-technical mailing list
Hi Swen,

On Mon, 11 Dec 2017 09:44:04 +0100, Swen Schillig via samba-technical
<[hidden email]> wrote:

> This is my first patch to SAMBA.
> I'm quiet sure it's easy stuff but I'm trying to gently introduce
> myself to the community.
> Please go easy on me if I failed already some basics at the beginning.

Most of this looks valid.  Can you please split it up into a commit per
logical change?  Putting each logical change into separate commit makes
things much clearer and also gives you more opportunity to explain each
change in more detail.

Comments:

* I don't think removing the talloc context argument is valid.  There
  is no guarantee that private_data points to a talloc context, so you
  can't depend on that even though it is true for every current use.  I
  can't imagine a circumstance where private_data is not a talloc
  context... but still...  :-)

* In the commit message for the destructor removal you could point to
  the commit that contains the removal of the only use of the
  "destroyed" structure member, which was really the only reason for
  the existence of the destructor.

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix wrong talloc context, remove queue_destructor and minor updates.

Samba - samba-technical mailing list
Hi Martin
On Tue, 2017-12-12 at 15:37 +1100, Martin Schwenke wrote:
> Hi Swen,
>
Thanks a lot for quick reply and the provided guidance.
> Comments:
>
> * I don't think removing the talloc context argument is valid.  There
>   is no guarantee that private_data points to a talloc context, so
you're right,
even though we might end up with a lot of other problems then.
Anyway, I removed that change.
>
> * In the commit message for the destructor removal you could point to
>   the commit that contains the removal of the only use of the
>   "destroyed" structure member, which was really the only reason for
>   the existence of the destructor.
Hmm, if I understand it correctly, then it was wrong even back then
when there was a consumer of "destroyed". Because right after calling
the destructor that attribute would even have been free'd as well,
right ?

I've split it all up and hopefully explained my intention.

I would really appreciate if you (and everybody else of course) could
have a look again.

Cheers Swen

patch.txt (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix wrong talloc context, remove queue_destructor and minor updates.

Samba - samba-technical mailing list
Hi Swen,

On Tue, Dec 12, 2017 at 9:18 PM, Swen Schillig via samba-technical
<[hidden email]> wrote:

> Hi Martin
> On Tue, 2017-12-12 at 15:37 +1100, Martin Schwenke wrote:
>> Hi Swen,
>>
> Thanks a lot for quick reply and the provided guidance.
>> Comments:
>>
>> * I don't think removing the talloc context argument is valid.  There
>>   is no guarantee that private_data points to a talloc context, so
> you're right,
> even though we might end up with a lot of other problems then.
> Anyway, I removed that change.
>>
>> * In the commit message for the destructor removal you could point to
>>   the commit that contains the removal of the only use of the
>>   "destroyed" structure member, which was really the only reason for
>>   the existence of the destructor.
> Hmm, if I understand it correctly, then it was wrong even back then
> when there was a consumer of "destroyed". Because right after calling
> the destructor that attribute would even have been free'd as well,
> right ?
>
> I've split it all up and hopefully explained my intention.
>
> I would really appreciate if you (and everybody else of course) could
> have a look again.

Patches look good, but I think they are incomplete.  The file
ctdb_io.c requires a bit of clean up.

I have abstracted the functionality of common/ctdb_io.c without
reference to ctdb_context in common/sock_io.c.  If you are interested
in improving the packet handling code, I would suggest you make the
relevant changes to common/sock_io.c. It already has some tests and
it's much easier to add tests for an abstraction that does not involve
ctdb_context.

The next step would be to simplify ctdb_io.c using sock_io
abstraction.  That way any improvements to sock_io are usable by ctdb
daemon and it avoids having to fix the code in two places.

Amitay.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix wrong talloc context, remove queue_destructor and minor updates.

Samba - samba-technical mailing list
On Thu, 2017-12-14 at 10:33 +1100, Amitay Isaacs wrote:

> Hi Swen,
>
> Patches look good, but I think they are incomplete.  The file
> ctdb_io.c requires a bit of clean up.
>
> I have abstracted the functionality of common/ctdb_io.c without
> reference to ctdb_context in common/sock_io.c.  If you are interested
> in improving the packet handling code, I would suggest you make the
> relevant changes to common/sock_io.c. It already has some tests and
> it's much easier to add tests for an abstraction that does not
> involve
> ctdb_context.
>
> The next step would be to simplify ctdb_io.c using sock_io
> abstraction.  That way any improvements to sock_io are usable by ctdb
> daemon and it avoids having to fix the code in two places.
>
> Amitay.
>
Thanks Amitay

I will do that...trying to get it done today.

Cheers Swen