Adding new MBIM extension CID?
Aleksander Morgado
aleksander at aleksander.es
Wed Apr 9 02:46:55 PDT 2014
On 09/04/14 10:23, Arnaud Desmier wrote:
> Can you point me what is wrong in this patch about coding guidelines and
> the other things?
Yes really sorry, should have done that already :) See below.
Well, the first thing is the API. I'm going to comment about it first of
all:
gboolean mbim_register_custom_service (
int service,
const MbimUuid *uuid);
gboolean mbim_unregister_custom_service (
int service);
You're giving yourself a custom number for the service ID. I think it
would be wise to let libmbim handle that itself, so instead we could
have this:
guint mbim_register_custom_service (
const MbimUuid *uuid,
const gchar *printable);
gboolean mbim_unregister_custom_service(
guint service);
So, you get the service ID directly as a guint, and you provide both the
uuid and the 'printable' name. The printable name should be used when
calling mbim_uuid_get_printable() or mbim_uuid_from_printable().
Internally, libmbim would keep track of which are the registered custom
services, and the match between service ID and uuid+printable.
Some general coding style things:
[1] The open-brace goes in the same line as the statement; e.g. like this:
if (something) {
...
}
not like this:
if (something)
{
}
[2] Whitespace is added before the open-parenthesis in method calls,
e.g. like this:
do_something ();
not like this:
do_something();
[3] Variables are declared at the beginning of each context, and a
whiteline is explicitly added between the variable declaration and the
first statement. E.g. like this:
guint foo;
do_something (&foo);
not like this:
guint foo;
do_something (&foo);
Now to the patch...
> From 23aa8d76ea320628cd455bdb1ff76759de829ad7 Mon Sep 17 00:00:00 2001
> From: adesmier <adesmier at sequans.com>
> Date: Fri, 21 Feb 2014 09:34:32 +0100
> Subject: [PATCH] Add support for custom services
>
> * use mbim_register_custom_service API to register a new UUID
> * use mbim_unregister_custom_service API unregister is
>
> * custom services are stored in a list
> ---
> src/libmbim-glib/mbim-message.c | 1 -
> src/libmbim-glib/mbim-uuid.c | 107 ++++++++++++++++++++++++++++++++++++++--
> src/libmbim-glib/mbim-uuid.h | 4 ++
> 3 files changed, 108 insertions(+), 4 deletions(-)
>
> diff --git a/src/libmbim-glib/mbim-message.c b/src/libmbim-glib/mbim-message.c
> index deb2c52..586a3e5 100644
> --- a/src/libmbim-glib/mbim-message.c
> +++ b/src/libmbim-glib/mbim-message.c
> @@ -1738,7 +1738,6 @@ mbim_message_command_new (guint32 transaction_id,
>
> /* Known service required */
> g_return_val_if_fail (service > MBIM_SERVICE_INVALID, FALSE);
> - g_return_val_if_fail (service <= MBIM_SERVICE_DSS, FALSE);
Whiteline here after g_return_val_if_fail()
> service_id = mbim_uuid_from_service (service);
>
> self = _mbim_message_allocate (MBIM_MESSAGE_TYPE_COMMAND,
> diff --git a/src/libmbim-glib/mbim-uuid.c b/src/libmbim-glib/mbim-uuid.c
> index 7c7160e..b49c4c1 100644
> --- a/src/libmbim-glib/mbim-uuid.c
> +++ b/src/libmbim-glib/mbim-uuid.c
> @@ -23,6 +23,7 @@
>
> #include <config.h>
> #include <stdio.h>
> +#include <stdlib.h>
> #include <string.h>
>
> #include "mbim-uuid.h"
> @@ -143,6 +144,15 @@ static const MbimUuid uuid_dss = {
> .e = { 0x6e, 0x0d, 0x58, 0x3c, 0x4d, 0x0e }
> };
>
> +struct MbimCustomService
> +{
> + int service_id;
> + const MbimUuid *uuid;
> + struct MbimCustomService *next;
> +};
> +
Instead of defining your custom list of structs with a next pointer;
just define a struct with service ID, uuid and printable string, and use
a GList to handle the list.
Also, service_id should be 'guint'.
And also, typedef the struct, so:
typedef struct {
...
} MbimCustomService;
And then use just "MbimCustomService", not "struct MbimCustomService".
> +static struct MbimCustomService *mbim_custom_service_list = NULL;
> +
> /**
> * mbim_uuid_from_service:
> * @service: a #MbimService.
> @@ -154,8 +164,7 @@ static const MbimUuid uuid_dss = {
> const MbimUuid *
> mbim_uuid_from_service (MbimService service)
> {
> - g_return_val_if_fail (service >= MBIM_SERVICE_INVALID && service <= MBIM_SERVICE_DSS,
> - &uuid_invalid);
> + g_return_val_if_fail (service >= MBIM_SERVICE_INVALID, &uuid_invalid);
>
> switch (service) {
> case MBIM_SERVICE_INVALID:
> @@ -175,7 +184,17 @@ mbim_uuid_from_service (MbimService service)
> case MBIM_SERVICE_DSS:
> return &uuid_dss;
> default:
> - g_assert_not_reached ();
> + {
Open-brace in the 'default' line.
> + struct MbimCustomService *current = mbim_custom_service_list;
Whiteline here.
> + while(current != NULL)
Space after while
> + {
Open-brace in the 'while' line.
> + if (current->service_id == service)
> + return current->uuid;
> + current = current->next;
> + }
> + g_warning("unknown service %i", service);
> + return &uuid_invalid;
> + }
> }
> }
>
> @@ -190,6 +209,8 @@ mbim_uuid_from_service (MbimService service)
> MbimService
> mbim_uuid_to_service (const MbimUuid *uuid)
> {
> + struct MbimCustomService *current = mbim_custom_service_list;
> +
GList *current...
> if (mbim_uuid_cmp (uuid, &uuid_basic_connect))
> return MBIM_SERVICE_BASIC_CONNECT;
>
> @@ -211,9 +232,89 @@ mbim_uuid_to_service (const MbimUuid *uuid)
> if (mbim_uuid_cmp (uuid, &uuid_dss))
> return MBIM_SERVICE_DSS;
>
> + while(current != NULL)
Whitespace after 'while'
> + {
open-brace in same line as 'while'.
> + if (current->uuid == uuid)
Use mbim_uuid_cmp() to compare uuids.
> + return current->service_id;
> + current = current->next;
> + }
> +
> return MBIM_SERVICE_INVALID;
> }
>
> +/**
> + * mbim_register_custom_service:
> + * @service: a new service ID, must be higher than MBIM_SERVICE_DSS
> + * @uuid: MbimUuid structure corresponding to service, pointer must be valid until service unregistration
> + *
> + * Register a custom service
> + *
> + * Return: TRUE if service has been registered, FALSE if not
> + */
> +gboolean mbim_register_custom_service(int service, const MbimUuid *uuid)
> +{
'gboolean' would be in its own line.
Also, whitespace before parenthesis.
> + struct MbimCustomService *current = mbim_custom_service_list;
> + struct MbimCustomService *n;
> +
> + int dss;
> + dss = MBIM_SERVICE_DSS;
> + g_return_val_if_fail (service > dss, FALSE);
With the API change suggested you're not passing the service ID
yourself, but anyway, the previous lines would just be:
g_return_val_if_fail (service > MBIM_SERVICE_DSS, FALSE);
i.e. no need to define an 'int dss' (which would actually be better
'guint dss').
> +
> + while (current != NULL)
> + {
Open-braces in the same line as 'while'
> + if (current->service_id == service)
> + {
Open-braces in the same line as 'if'
> + g_warning("service %i already registered", service);
Whitespace before parenthesis.
> + return FALSE;
> + }
> + current = current->next;
> + }
Anyway, this loop would not be needed; as you wouldn't be passing the
service ID in the suggested API change. Instead, there should be a loop
to look for an unused service ID to use for the newly registered service.
You can e.g. start the registered service IDs with number '100'.
> +
> + n = (struct MbimCustomService *)malloc(sizeof(struct MbimCustomService));
> + if (!n)
> + g_error("Failed to allocate MbimCustomService"); // Fatal
No malloc; use g_malloc() or g_new() or even better, g_slice_new(); i.e.
n = g_slice_new (MbimCustomService);
BTW; no need to check for heap allocation failures with g_malloc(),
g_new() or g_slice_new(); GLib will just g_error() itself internally if
that happens.
> +
> + n->service_id = service;
> + n->uuid = uuid;
Would be good to make a copy of the uuid ourselves and store it here. We
shouldn't rely on the caller to keep the uuid pointer valid. And at the
end, comparing with mbim_uuid_cmp() compares the contents of the uuid,
not the pointers.
So instead of adding a "const MbimUuid *" in the struct, add a
"MbimUuid", and memcpy the contents of the received uuid into the uuid
within the struct.
> + n->next = mbim_custom_service_list;
> + mbim_custom_service_list = n;
As previously said, a GList to handle all these.
> + g_message("service %i registered", service);
Whitespace before parenthesis.
> +
> + return TRUE;
> +}
> +
> +/**
> + * mbim_unregister_custom_service:
> + * @service: custom service ID to remove from custom list
> + *
> + * Unregister a custom service
> + *
> + * Return: TRUE if service has been unregistered, FALSE if service is not found
> + */
> +gboolean mbim_unregister_custom_service(int service)
'gboolean' in its own line.
Also, whitespace before parenthesis.
> +{
> + struct MbimCustomService *current = mbim_custom_service_list;
> + struct MbimCustomService *prev = NULL;
> +
> + g_return_val_if_fail (service > MBIM_SERVICE_DSS, FALSE);
> +
> + while (current != NULL)
> + {
Open-braces in the same line as the 'while'.
> + if (current->service_id == service)
> + {
Open-braces in the same line as the 'if'.
> + if (prev)
> + prev->next = current->next;
> + else
> + mbim_custom_service_list = current->next;
> + free(current);
> + return TRUE;
> + }
> + prev = current;
> + current = current->next;
> + }
> + return FALSE;
When using GList for the list and g_slice_new() for the allocation, this
would look like:
GList *l;
for (l = mbim_custom_service_list; l; l = g_list_next (l)) {
if (((MbimCustomService *)l->data)->service_id == service) {
g_slice_free (MbimCustomService, l->data);
mbim_custom_service_list = \
g_list_delete_link (mbim_custom_service_list, l);
return TRUE;
}
}
return FALSE;
> +}
> +
> /*****************************************************************************/
>
> static const MbimUuid uuid_context_type_none = {
> diff --git a/src/libmbim-glib/mbim-uuid.h b/src/libmbim-glib/mbim-uuid.h
> index 0676861..de9695d 100644
> --- a/src/libmbim-glib/mbim-uuid.h
> +++ b/src/libmbim-glib/mbim-uuid.h
> @@ -156,6 +156,10 @@ typedef enum {
> const MbimUuid *mbim_uuid_from_service (MbimService service);
> MbimService mbim_uuid_to_service (const MbimUuid *uuid);
>
> +// create a new custom service
> +gboolean mbim_register_custom_service(int service, const MbimUuid *uuid);
> +gboolean mbim_unregister_custom_service(int service);
Whitespaces before the parenthesis.
> +
> /*****************************************************************************/
>
> /**
> --
> 1.8.3.2
>
Cheers!
--
Aleksander
https://aleksander.es
More information about the libmbim-devel
mailing list