[PATCH] Emit missing signal when data channel is reconnected.

[PATCH] Emit missing signal when data channel is reconnected.

[Santiago Carot-Nemesio]

Reconnections of data channels should be indicated to others
applications by using the appropriate signal.
---
 health/hdp.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/health/hdp.c b/health/hdp.c
index 3c2dce1..b06fe17 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -510,14 +510,23 @@ static void hdp_mdl_reconn_cb(struct mcap_mdl *mdl, GError *err, gpointer data)
 	}
 
 	fd = mcap_mdl_get_fd(dc_data->hdp_chann->mdl);
-	if (fd < 0)
+	if (fd < 0) {
 		reply = g_dbus_create_error(dc_data->msg,
 						ERROR_INTERFACE ".HealthError",
 						"Cannot get file descriptor");
+		g_dbus_send_message(dc_data->conn, reply);
+		return;
+	}
 
 	reply = g_dbus_create_reply(dc_data->msg, DBUS_TYPE_UNIX_FD, &fd,
 							DBUS_TYPE_INVALID);
 	g_dbus_send_message(dc_data->conn, reply);
+
+	g_dbus_emit_signal(dc_data->conn,
+			device_get_path(dc_data->hdp_chann->dev->dev),
+			HEALTH_DEVICE, "ChannelConnected",
+			DBUS_TYPE_OBJECT_PATH,
+			&dc_data->hdp_chann->path, DBUS_TYPE_INVALID);
 }
 
 static void hdp_get_dcpsm_cb(uint16_t dcpsm, gpointer user_data, GError *err)
-- 
1.7.4.1

Re: [PATCH] Emit missing signal when data channel is reconnected.

[Johan Hedberg]

Hi,

On Fri, Mar 25, 2011, Santiago Carot-Nemesio wrote:
> Reconnections of data channels should be indicated to others
> applications by using the appropriate signal.
> ---
>  health/hdp.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/health/hdp.c b/health/hdp.c
> index 3c2dce1..b06fe17 100644
> --- a/health/hdp.c
> +++ b/health/hdp.c
> @@ -510,14 +510,23 @@ static void hdp_mdl_reconn_cb(struct mcap_mdl *mdl, GError *err, gpointer data)
>  	}
>  
>  	fd = mcap_mdl_get_fd(dc_data->hdp_chann->mdl);
> -	if (fd < 0)
> +	if (fd < 0) {
>  		reply = g_dbus_create_error(dc_data->msg,
>  						ERROR_INTERFACE ".HealthError",
>  						"Cannot get file descriptor");
> +		g_dbus_send_message(dc_data->conn, reply);
> +		return;
> +	}
>  
>  	reply = g_dbus_create_reply(dc_data->msg, DBUS_TYPE_UNIX_FD, &fd,
>  							DBUS_TYPE_INVALID);
>  	g_dbus_send_message(dc_data->conn, reply);
> +
> +	g_dbus_emit_signal(dc_data->conn,
> +			device_get_path(dc_data->hdp_chann->dev->dev),
> +			HEALTH_DEVICE, "ChannelConnected",
> +			DBUS_TYPE_OBJECT_PATH,
> +			&dc_data->hdp_chann->path, DBUS_TYPE_INVALID);
>  }
>  
>  static void hdp_get_dcpsm_cb(uint16_t dcpsm, gpointer user_data, GError *err)

Looks like this patch is doing two things:

1. Fix the memory leak/missing g_dbus_send_message call for the return
value from g_dbus_create_error.

2. Add sending of the "ChannelConnected" signal.

Could you please split it into two separate patches?

Johan

Re: [PATCH] Emit missing signal when data channel is reconnected.

[Santiago Carot]

Hi Johan,

See coments below.

2011/3/25 Johan Hedberg <johan.hedberg@gmail.com>:
> Hi,
>
> On Fri, Mar 25, 2011, Santiago Carot-Nemesio wrote:
>> Reconnections of data channels should be indicated to others
>> applications by using the appropriate signal.
>> ---
>>  health/hdp.c |   11 ++++++++++-
>>  1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/health/hdp.c b/health/hdp.c
>> index 3c2dce1..b06fe17 100644
>> --- a/health/hdp.c
>> +++ b/health/hdp.c
>> @@ -510,14 +510,23 @@ static void hdp_mdl_reconn_cb(struct mcap_mdl *mdl, GError *err, gpointer data)
>>       }
>>
>>       fd = mcap_mdl_get_fd(dc_data->hdp_chann->mdl);
>> -     if (fd < 0)
>> +     if (fd < 0) {
>>               reply = g_dbus_create_error(dc_data->msg,
>>                                               ERROR_INTERFACE ".HealthError",
>>                                               "Cannot get file descriptor");
>> +             g_dbus_send_message(dc_data->conn, reply);
>> +             return;
>> +     }
>>

This is not a memory leak fix, if fd is less than 0, then reconnection
was not succesfully and then we reply with an error response to the
application but we dont not emit the CahnnelConnected signal. (return
statement).

>>       reply = g_dbus_create_reply(dc_data->msg, DBUS_TYPE_UNIX_FD, &fd,
>>                                                       DBUS_TYPE_INVALID);
>>       g_dbus_send_message(dc_data->conn, reply);

At this point we have a valid file descriptor and we can send the signal.

>> +
>> +     g_dbus_emit_signal(dc_data->conn,
>> +                     device_get_path(dc_data->hdp_chann->dev->dev),
>> +                     HEALTH_DEVICE, "ChannelConnected",
>> +                     DBUS_TYPE_OBJECT_PATH,
>> +                     &dc_data->hdp_chann->path, DBUS_TYPE_INVALID);
>>  }
>>
>>  static void hdp_get_dcpsm_cb(uint16_t dcpsm, gpointer user_data, GError *err)
>
> Looks like this patch is doing two things:
>
> 1. Fix the memory leak/missing g_dbus_send_message call for the return
> value from g_dbus_create_error.
>
> 2. Add sending of the "ChannelConnected" signal.
>
> Could you please split it into two separate patches?
>
> Johan
>

Re: [PATCH] Emit missing signal when data channel is reconnected.

[Johan Hedberg]

Hi,

On Fri, Mar 25, 2011, Santiago Carot wrote:
> >> diff --git a/health/hdp.c b/health/hdp.c
> >> index 3c2dce1..b06fe17 100644
> >> --- a/health/hdp.c
> >> +++ b/health/hdp.c
> >> @@ -510,14 +510,23 @@ static void hdp_mdl_reconn_cb(struct mcap_mdl *mdl, GError *err, gpointer data)
> >>       }
> >>
> >>       fd = mcap_mdl_get_fd(dc_data->hdp_chann->mdl);
> >> -     if (fd < 0)
> >> +     if (fd < 0) {
> >>               reply = g_dbus_create_error(dc_data->msg,
> >>                                               ERROR_INTERFACE ".HealthError",
> >>                                               "Cannot get file descriptor");
> >> +             g_dbus_send_message(dc_data->conn, reply);
> >> +             return;
> >> +     }
> >>
> 
> This is not a memory leak fix, if fd is less than 0, then reconnection
> was not succesfully and then we reply with an error response to the
> application but we dont not emit the CahnnelConnected signal. (return
> statement).

The code (before your patch) was assigning a value to reply and then
right afterwards assigning another value to it without freeing it in
between:

> >>       reply = g_dbus_create_reply(dc_data->msg, DBUS_TYPE_UNIX_FD, &fd,
> >>                                                       DBUS_TYPE_INVALID);

How is that not a memory leak? (not to mention that g_dbus_send_message
isn't called to the error message that you create). Looks to me like
there was an "else" statement missing or something similar.

Johan

Re: [PATCH] Emit missing signal when data channel is reconnected.

[Santiago Carot]

Hi,

2011/3/25 Johan Hedberg <johan.hedberg@gmail.com>:
> Hi,
>
> On Fri, Mar 25, 2011, Santiago Carot wrote:
>> >> diff --git a/health/hdp.c b/health/hdp.c
>> >> index 3c2dce1..b06fe17 100644
>> >> --- a/health/hdp.c
>> >> +++ b/health/hdp.c
>> >> @@ -510,14 +510,23 @@ static void hdp_mdl_reconn_cb(struct mcap_mdl *mdl, GError *err, gpointer data)
>> >>       }
>> >>
>> >>       fd = mcap_mdl_get_fd(dc_data->hdp_chann->mdl);
>> >> -     if (fd < 0)
>> >> +     if (fd < 0) {
>> >>               reply = g_dbus_create_error(dc_data->msg,
>> >>                                               ERROR_INTERFACE ".HealthError",
>> >>                                               "Cannot get file descriptor");
>> >> +             g_dbus_send_message(dc_data->conn, reply);
>> >> +             return;
>> >> +     }
>> >>
>>
>> This is not a memory leak fix, if fd is less than 0, then reconnection
>> was not succesfully and then we reply with an error response to the
>> application but we dont not emit the CahnnelConnected signal. (return
>> statement).
>
> The code (before your patch) was assigning a value to reply and then
> right afterwards assigning another value to it without freeing it in
> between:
>
>> >>       reply = g_dbus_create_reply(dc_data->msg, DBUS_TYPE_UNIX_FD, &fd,
>> >>                                                       DBUS_TYPE_INVALID);
>
> How is that not a memory leak? (not to mention that g_dbus_send_message
> isn't called to the error message that you create). Looks to me like
> there was an "else" statement missing or something similar.
>

Sorry,
I didn't understood what you meant. I though that you was referring to
the pach but you was referring to the old code. Now I see.
You are right, I'm going to split it in two patches right away.