Re: [PATCH 4/4] Sim Access Profile dummy driver

[Johan Hedberg <johan.hedberg@gmail.com>]

Hi Waldek,

On Wed, Nov 03, 2010, Waldemar Rymarkiewicz wrote:
> +static sim_connection_status_t sim_card_connection_status = SIM_DISCONNECTED;

Consider shortening these names. At least one of them (either the type
or the variable).

> +static void *		sap_data = NULL;  /* SAP server private data.*/

No whitespace between * and sap_data. Why is this void instead of a
specific type when its static and therefore wont be seen by other
modules?

> +	if(sim_card_connection_status == SIM_MISSING)

Space before (

> +	}else if(sim_card_connection_status != SIM_CONNECTED)

Space after } and before (

> +		sap_power_sim_on_rsp(sap_device, SAP_RESULT_ERROR_NOT_ACCESSIBLE);
> +	else {
> +		sap_power_sim_on_rsp(sap_device, SAP_RESULT_ERROR_NO_REASON);
> +	}

Why do you have {} for one branch but not for the other? In general
one-line branches/scopes shouldn't have braces, though the kernel coding
guideline does allow them if at least one part of the same if-else
statement has them.

> +static inline DBusMessage *invalid_args(DBusMessage *msg)

Never mind the previous comment I had about this. I checked the rest of
the tree and it seems this inline convention for D-Bus error generating
functions is quite common (though imho might not be needed at all).

> +	if(sim_card_connection_status != SIM_CONNECTED)

Space before (

> +}
> +
> +
> +static GDBusMethodTable dummy_methods[] = {

Remove the other empty line.

> +	{ "OngoingCall",	"b",	"",	ongoing_call},
> +	{ "MaxMessageSize",	"u",	"",	max_msg_size},
> +	{ "Disconnect",		"",	"",	disconnect},
> +	{ "CardStatus",		"u",	"",	card_status},
> +	{ }

> +static GDBusSignalTable dummy_signals[] = {
> +	{ "","" },
> +	{ }
> +};

If you have no signals just pass NULL to g_dbus_register_interface
instead of declaring an (almost) empty table. Also, "" isn't a valid
value for the signal name.

Johan