[PATCH 1/2] Do not increment mdl reference in reconnections

[PATCH 1/2] Do not increment mdl reference in reconnections

[Santiago Carot-Nemesio]

diff --git a/health/hdp.c b/health/hdp.c
index 2e06a6b..769e300 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -921,7 +921,8 @@ static void hdp_mcap_mdl_connected_cb(struct mcap_mdl *mdl, void *data)
 		return;
 
 	chan = dev->ndc;
-	chan->mdl = mcap_mdl_ref(mdl);
+	if (!chan->mdl)
+		chan->mdl = mcap_mdl_ref(mdl);
 
 	if (!g_slist_find(dev->channels, chan))
 		dev->channels = g_slist_prepend(dev->channels,
-- 
1.7.3.2

[PATCH 2/2] Increment reference counter when a io_watcher is added

[Jose Antonio Santos Cadenas]

Also a destroy function is added in order to avoid memory leaks
---
 health/mcap.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/health/mcap.c b/health/mcap.c
index 8ecdc68..81fd8df 100644
--- a/health/mcap.c
+++ b/health/mcap.c
@@ -1638,8 +1638,11 @@ static void mcap_connect_mdl_cb(GIOChannel *chan, GError *conn_err,
 	}
 
 	mdl->state = MDL_CONNECTED;
-	mdl->wid = g_io_add_watch(mdl->dc, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
-						(GIOFunc) mdl_event_cb, mdl);
+	mdl->wid = g_io_add_watch_full(mdl->dc, G_PRIORITY_DEFAULT,
+					G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+					(GIOFunc) mdl_event_cb,
+					mcap_mdl_ref(mdl),
+					(GDestroyNotify) mcap_mdl_unref);
 
 	cb(mdl, conn_err, user_data);
 }
@@ -1774,9 +1777,11 @@ static void mcap_connect_mcl_cb(GIOChannel *chan, GError *conn_err,
 							mcap_mcl_ref(mcl));
 	}
 
-	mcl->wid = g_io_add_watch(mcl->cc,
+	mcl->wid = g_io_add_watch_full(mcl->cc, G_PRIORITY_DEFAULT,
 				G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
-				(GIOFunc) mcl_control_cb, mcl);
+				(GIOFunc) mcl_control_cb,
+				mcap_mcl_ref(mcl),
+				(GDestroyNotify) mcap_mcl_unref);
 	connect_cb(mcl, gerr, data);
 }
 
@@ -1786,8 +1791,11 @@ static void set_mdl_properties(GIOChannel *chan, struct mcap_mdl *mdl)
 
 	mdl->state = MDL_CONNECTED;
 	mdl->dc = g_io_channel_ref(chan);
-	mdl->wid = g_io_add_watch(mdl->dc, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
-						(GIOFunc) mdl_event_cb, mdl);
+	mdl->wid = g_io_add_watch_full(mdl->dc, G_PRIORITY_DEFAULT,
+					G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+					(GIOFunc) mdl_event_cb,
+					mcap_mdl_ref(mdl),
+					(GDestroyNotify) mcap_mdl_unref);
 
 	mcl->state = MCL_ACTIVE;
 	mcl->cb->mdl_connected(mdl, mcl->cb->user_data);
@@ -1919,9 +1927,11 @@ static void set_mcl_conf(GIOChannel *chan, struct mcap_mcl *mcl)
 		mcl->mi->mcls = g_slist_prepend(mcl->mi->mcls,
 							mcap_mcl_ref(mcl));
 
-	mcl->wid = g_io_add_watch(mcl->cc,
-			G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
-			(GIOFunc) mcl_control_cb, mcl);
+	mcl->wid = g_io_add_watch_full(mcl->cc, G_PRIORITY_DEFAULT,
+				G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+				(GIOFunc) mcl_control_cb,
+				mcap_mcl_ref(mcl),
+				(GDestroyNotify) mcap_mcl_unref);
 
 	/* Callback to report new MCL */
 	if (reconn)
-- 
1.7.1

Re: [PATCH 2/2] Increment reference counter when a io_watcher is added

[Johan Hedberg]

Hi Jose,

On Mon, Nov 22, 2010, Jose Antonio Santos Cadenas wrote:
> Also a destroy function is added in order to avoid memory leaks
> ---
>  health/mcap.c |   28 +++++++++++++++++++---------
>  1 files changed, 19 insertions(+), 9 deletions(-)

The patch content itself is fine but you need to fix the commit message.
Now you have half of the info in the summary and half in the message
body (both halves being equaly relevant). The summary line should be a
summary of the whole patch (in this case it could be e.g. "Fix MDL
reference counting for IO watchers") and the body should contain the
details. So please reformulate, resend, and pay more attention to this
kind of things in the future. Thanks :)

Johan

Re: [PATCH 2/2] Increment reference counter when a io_watcher is added

[Jose Antonio Santos Cadenas]

Hi Johan,

2010/11/22 Johan Hedberg <johan.hedberg@gmail.com>:
> Hi Jose,
>
> On Mon, Nov 22, 2010, Jose Antonio Santos Cadenas wrote:
>> Also a destroy function is added in order to avoid memory leaks
>> ---
>>  health/mcap.c |   28 +++++++++++++++++++---------
>>  1 files changed, 19 insertions(+), 9 deletions(-)
>
> The patch content itself is fine but you need to fix the commit message.
> Now you have half of the info in the summary and half in the message
> body (both halves being equaly relevant). The summary line should be a
> summary of the whole patch (in this case it could be e.g. "Fix MDL
> reference counting for IO watchers") and the body should contain the
> details. So please reformulate, resend, and pay more attention to this
> kind of things in the future. Thanks :)

Sorry for for the inconvenience, I'm fixing this and sending the new
patch. I'll try be more carefully with the commit messages in the
future.

Regards

>
> Johan
>

[PATCH 2/2] Fix: increment MDL and MCL reference counter in IO watchers.

[Jose Antonio Santos Cadenas]

When a io_watcher is added to an MDL or an MCL channel, its reference
should be incremented because the watcher should keep its own
reference the the structure.

Also  a destroy function is added in order to decrement the reference
once the watcher is removed.
---
 health/mcap.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/health/mcap.c b/health/mcap.c
index 8ecdc68..81fd8df 100644
--- a/health/mcap.c
+++ b/health/mcap.c
@@ -1638,8 +1638,11 @@ static void mcap_connect_mdl_cb(GIOChannel *chan, GError *conn_err,
 	}
 
 	mdl->state = MDL_CONNECTED;
-	mdl->wid = g_io_add_watch(mdl->dc, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
-						(GIOFunc) mdl_event_cb, mdl);
+	mdl->wid = g_io_add_watch_full(mdl->dc, G_PRIORITY_DEFAULT,
+					G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+					(GIOFunc) mdl_event_cb,
+					mcap_mdl_ref(mdl),
+					(GDestroyNotify) mcap_mdl_unref);
 
 	cb(mdl, conn_err, user_data);
 }
@@ -1774,9 +1777,11 @@ static void mcap_connect_mcl_cb(GIOChannel *chan, GError *conn_err,
 							mcap_mcl_ref(mcl));
 	}
 
-	mcl->wid = g_io_add_watch(mcl->cc,
+	mcl->wid = g_io_add_watch_full(mcl->cc, G_PRIORITY_DEFAULT,
 				G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
-				(GIOFunc) mcl_control_cb, mcl);
+				(GIOFunc) mcl_control_cb,
+				mcap_mcl_ref(mcl),
+				(GDestroyNotify) mcap_mcl_unref);
 	connect_cb(mcl, gerr, data);
 }
 
@@ -1786,8 +1791,11 @@ static void set_mdl_properties(GIOChannel *chan, struct mcap_mdl *mdl)
 
 	mdl->state = MDL_CONNECTED;
 	mdl->dc = g_io_channel_ref(chan);
-	mdl->wid = g_io_add_watch(mdl->dc, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
-						(GIOFunc) mdl_event_cb, mdl);
+	mdl->wid = g_io_add_watch_full(mdl->dc, G_PRIORITY_DEFAULT,
+					G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+					(GIOFunc) mdl_event_cb,
+					mcap_mdl_ref(mdl),
+					(GDestroyNotify) mcap_mdl_unref);
 
 	mcl->state = MCL_ACTIVE;
 	mcl->cb->mdl_connected(mdl, mcl->cb->user_data);
@@ -1919,9 +1927,11 @@ static void set_mcl_conf(GIOChannel *chan, struct mcap_mcl *mcl)
 		mcl->mi->mcls = g_slist_prepend(mcl->mi->mcls,
 							mcap_mcl_ref(mcl));
 
-	mcl->wid = g_io_add_watch(mcl->cc,
-			G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
-			(GIOFunc) mcl_control_cb, mcl);
+	mcl->wid = g_io_add_watch_full(mcl->cc, G_PRIORITY_DEFAULT,
+				G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+				(GIOFunc) mcl_control_cb,
+				mcap_mcl_ref(mcl),
+				(GDestroyNotify) mcap_mcl_unref);
 
 	/* Callback to report new MCL */
 	if (reconn)
-- 
1.7.1

Re: [PATCH 2/2] Fix: increment MDL and MCL reference counter in IO watchers.

[Johan Hedberg]

Hi Jose,

On Mon, Nov 22, 2010, Jose Antonio Santos Cadenas wrote:
> When a io_watcher is added to an MDL or an MCL channel, its reference
> should be incremented because the watcher should keep its own
> reference the the structure.
> 
> Also  a destroy function is added in order to decrement the reference
> once the watcher is removed.
> ---
>  health/mcap.c |   28 +++++++++++++++++++---------
>  1 files changed, 19 insertions(+), 9 deletions(-)

Thanks. Pushed upstream, but I still had to tweak the summary line a
little bit (take a look at the upstream tree if you're interested).

Johan

Re: [PATCH 2/2] Fix: increment MDL and MCL reference counter in IO watchers.

[Jose Antonio Santos Cadenas]

2010/11/22 Johan Hedberg <johan.hedberg@gmail.com>:
> Hi Jose,
>
> On Mon, Nov 22, 2010, Jose Antonio Santos Cadenas wrote:
>> When a io_watcher is added to an MDL or an MCL channel, its reference
>> should be incremented because the watcher should keep its own
>> reference the the structure.
>>
>> Also  a destroy function is added in order to decrement the reference
>> once the watcher is removed.
>> ---
>>  health/mcap.c |   28 +++++++++++++++++++---------
>>  1 files changed, 19 insertions(+), 9 deletions(-)
>
> Thanks. Pushed upstream, but I still had to tweak the summary line a
> little bit (take a look at the upstream tree if you're interested).

I have to avoid dots at the end of the commit message I always think
that, but sometimes my fingers are quicker than my mind. Sorry :) .

>
> Johan
>

Re: [PATCH 1/2] Do not increment mdl reference in reconnections

[Johan Hedberg]

Hi,

On Mon, Nov 22, 2010, Santiago Carot-Nemesio wrote:
> diff --git a/health/hdp.c b/health/hdp.c
> index 2e06a6b..769e300 100644
> --- a/health/hdp.c
> +++ b/health/hdp.c
> @@ -921,7 +921,8 @@ static void hdp_mcap_mdl_connected_cb(struct mcap_mdl *mdl, void *data)
>  		return;
>  
>  	chan = dev->ndc;
> -	chan->mdl = mcap_mdl_ref(mdl);
> +	if (!chan->mdl)
> +		chan->mdl = mcap_mdl_ref(mdl);
>  
>  	if (!g_slist_find(dev->channels, chan))
>  		dev->channels = g_slist_prepend(dev->channels,

Pushed upstream. Do try to be more verbose in your commit messages
please. Also have the summary lines of fix commits (like this one)
starting with "Fix ..." so they can be easily detected e.g. when doing
summaries for release changelogs.

Johan