Patch for Phonebook CLI
Aleksander Morgado
aleksander at lanedo.com
Mon Oct 7 23:46:28 PDT 2013
Hey Raju!
Please find inline below several comments to address.
It overall looks good, just some changes needed in some actions and
several coding style changes. For the coding style changes; please
review each of the comments and apply the fix everywhere in the patch; I
only commented each coding style fix once, even if it appeared multiple
times. Getting the coding style right is very important so that we can
focus only on the actual logic being implemented while reviewing.
Thanks for this effort, and looking forward for a v2 patch with the fixes :)
>
>
> From fc6ec874481645be7b78201b08c503725b2fedc5 Mon Sep 17 00:00:00 2001
> From: root <root at ubuntu.ubuntu-domain>
Please configure git properly so that your full name and email address
is included in the patch.
> Date: Mon, 7 Oct 2013 20:11:38 +0530
> Subject: [PATCH] Phonebook updated
Please follow the style of previous commit messages, e.g.:
mbimcli: implement phonebook support
[Plus more description if needed]
>
> ---
> cli/Makefile.am | 3 +-
> cli/mbimcli-phonebook.c | 412 +++++++++++++++++++++++++++++++++++++++++++++++
> cli/mbimcli.c | 9 ++
> cli/mbimcli.h | 4 +
> 4 files changed, 427 insertions(+), 1 deletion(-)
> create mode 100644 cli/mbimcli-phonebook.c
>
> diff --git a/cli/Makefile.am b/cli/Makefile.am
> index 3685e93..9c70fa2 100644
> --- a/cli/Makefile.am
> +++ b/cli/Makefile.am
> @@ -12,7 +12,8 @@ mbimcli_CPPFLAGS = \
> mbimcli_SOURCES = \
> mbimcli.h \
> mbimcli.c \
> - mbimcli-basic-connect.c
> + mbimcli-basic-connect.c \
> + mbimcli-phonebook.c
>
> mbimcli_LDADD = \
> $(MBIMCLI_LIBS) \
> diff --git a/cli/mbimcli-phonebook.c b/cli/mbimcli-phonebook.c
> new file mode 100644
> index 0000000..2d3aff0
> --- /dev/null
> +++ b/cli/mbimcli-phonebook.c
> @@ -0,0 +1,412 @@
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/*
> + * mbimcli -- Command line interface to control MBIM devices
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (C) 2013 Aleksander Morgado <aleksander at gnu.org>
Ha, LOL :) I don't remember having written this file. You should put
your own copyright line in here.
> + */
> +
> +#include "config.h"
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <locale.h>
> +#include <string.h>
> +
> +#include <glib.h>
> +#include <gio/gio.h>
> +
> +#include <libmbim-glib.h>
> +
> +#include "mbimcli.h"
Leave a white line here.
> +/* Context */
> +typedef struct {
> + MbimDevice *device;
> + GCancellable *cancellable;
> +} Context;
> +static Context *ctx;
> +
> +static gboolean phonebook_configuration_flag;
> +static gboolean phonebook_read_flag;
> +static gboolean phonebook_read_all_flag;
> +static gboolean phonebook_write_flag;
> +static gboolean phonebook_delete_flag;
> +static gboolean phonebook_delete_all_flag;
> +static gchar *phonebook_name = NULL;
> +static gchar *phonebook_number= NULL;
> +static gint phonebook_index = 0;
> +static GOptionEntry entries[] = {
> + { "phonebook-configuration", 0, 0, G_OPTION_ARG_NONE, &phonebook_configuration_flag,
> + "phonebook configuration",
> + NULL
> + },
I would call this action "phonebook-query-configuration". Also, the
explanation of the action should be better defined, something like
"Query the phonebook configuration".
> + { "phonebook-read", 0, 0, G_OPTION_ARG_NONE, &phonebook_read_flag,
> + "phonebook read",
> + NULL
> + },
Reading a phonebook entry requires and index; and you should pass that
index directly in this action as an argument, not passed with another
option afterwards. The action should be --phonebook-read=[(index)]. See
e.g. how it's done in basic-connect's --enter-pin action.
> + { "phonebook-read-all", 0, 0, G_OPTION_ARG_NONE, &phonebook_read_all_flag,
> + "phonebook read all",
> + NULL
> + },
Ok.
> + { "phonebook-write", 0, 0, G_OPTION_ARG_NONE, &phonebook_write_flag,
> + "phonebook write",
> + NULL
> + },
The action should be --phonebook-write=[(number),(name)]. This is, the
action should receive a string with a comma-separated list of values.
See e.g. how basic-connect's --connect action does it.
> + { "phonebook-delete", 0, 0, G_OPTION_ARG_NONE, &phonebook_delete_flag,
> + "phonebook delete",
> + NULL
> + },
Same comments as for --phonebook-read; it should be
--phonebook-delete=[(index)].
> + { "phonebook-delete-all", 0, 0, G_OPTION_ARG_NONE, &phonebook_delete_all_flag,
> + "phonebook delete all",
> + NULL
> + },
Ok.
> + { "phonebook-name", 0, 0, G_OPTION_ARG_STRING, &phonebook_name,
> + "name to add in phonebook",
> + NULL
> + },
> + { "phonebook-number", 0, 0, G_OPTION_ARG_STRING, &phonebook_number,
> + "number to add in phonebook",
> + NULL
> + },
> + { "phonebook-index", 0, 0, G_OPTION_ARG_INT, &phonebook_index,
> + "index to delete phonebook entry",
> + NULL
> + },
These previous three options therefore will not be needed. We should
ideally only have options for actions, not for additional arguments to pass.
> + { NULL }
> +};
> +
> +GOptionGroup *
> +mbimcli_phonebook_get_option_group (void)
> +{
> + GOptionGroup *group;
> + group = g_option_group_new ("phonebook",
> + "Phonebook options",
> + "Show Phonebook Service options",
> + NULL,
> + NULL);
> + g_option_group_add_entries (group, entries);
> +
> + return group;
> +}
> +
> +gboolean
> +mbimcli_phonebook_options_enabled (void)
> +{
> + static guint n_actions = 0;
> + static gboolean checked = FALSE;
Always leave a whiteline between the last declared value and the first
method call.
NOTE: This comment is applicable to most of the functions in the file,
so please fix all the other ones as well.
> + if (checked)
> + return !!n_actions;
> +
> + n_actions = (phonebook_configuration_flag +
> + phonebook_read_flag +
> + phonebook_read_all_flag +
> + phonebook_write_flag +
> + phonebook_delete_flag +
> + phonebook_delete_all_flag );
Tabs should be used neither for indentation nor for alignment. Use
always spaces.
> +
> + if (n_actions > 1) {
> + g_printerr ("error: too many phonebook actions requested\n");
> + exit (EXIT_FAILURE);
> + }
> +
> + checked = TRUE;
> + return !!n_actions;
> +}
> +
> +static void
> +context_free (Context *context)
> +{
> + if (!context)
> + return;
> +
> + if (context->cancellable)
> + g_object_unref (context->cancellable);
> + if (context->device)
> + g_object_unref (context->device);
> + g_slice_free (Context, context);
> +}
> +
> +static void
> +shutdown (gboolean operation_status)
> +{
> + /* Cleanup context and finish async operation */
> + context_free (ctx);
> + mbimcli_async_operation_done (operation_status);
> +}
> +
> +static void
> +set_phonebook_write_ready (MbimDevice *device,
> + GAsyncResult *res)
Need alignment of parameters here.
NOTE: This comment is applicable to most of the functions in the file,
so please fix all the other ones as well.
> +{
> + MbimMessage *response;
> + GError *error = NULL;
> + response = mbim_device_command_finish (device, res, &error);
> + if (!response) {
> + g_printerr ("error: operation failed: %s\n", error->message);
> + g_error_free (error);
> + shutdown (FALSE);
> + return;
> + }
> +
> + if(!mbim_message_phonebook_write_response_parse (
> + response,
> + &error)){
> + g_printerr ("error: couldn't parse response message: %s\n", error->message);
> + g_error_free (error);
> + shutdown (FALSE);
> + return;
> + }
When the action succeeds, you should print a message stating it; e.g.
g_print ("New phonebook entry correctly written\n");
NOTE: This comment is applicable to most of the functions in the file,
so please fix all the other ones as well.
> + mbim_message_unref (response);
> + shutdown (TRUE);
> +
> + return;
> +}
> +
> +static void
> +set_phonebook_delete_ready (MbimDevice *device,
> + GAsyncResult *res)
> +{
> + MbimMessage *response;
> + GError *error = NULL;
> + response = mbim_device_command_finish (device, res, &error);
> + if (!response) {
> + g_printerr ("error: operation failed: %s\n", error->message);
> + g_error_free (error);
> + shutdown (FALSE);
> + return;
> + }
> +
> + if(!mbim_message_phonebook_delete_response_parse (
> + response,
> + &error)){
> + g_printerr ("error: couldn't parse response message: %s\n", error->message);
> + g_error_free (error);
> + shutdown (FALSE);
> + return;
> + }
> +
> + mbim_message_unref (response);
> + shutdown (TRUE);
> + return;
> +}
> +
> +static void
> +query_phonebook_read_ready (MbimDevice *device,
> + GAsyncResult *res)
> +{
> + MbimMessage *response;
> + GError *error = NULL;
> + guint32 entry_count;
> + MbimPhonebookEntry **phonebook_entries;
> + gint i = 0;
> + response = mbim_device_command_finish (device, res, &error);
> + if (!response) {
> + g_printerr ("error: operation failed: %s\n", error->message);
> + g_error_free (error);
> + shutdown (FALSE);
> + return;
> + }
> +
> + if(!mbim_message_phonebook_read_response_parse (
> + response,
> + &entry_count,
> + &phonebook_entries,
> + &error)){
> + g_printerr ("error: couldn't parse response message: %s\n", error->message);
> + g_error_free (error);
> + shutdown (FALSE);
> + return;
> + }
> +
> +#undef VALIDATE_UNKNOWN
> +#define VALIDATE_UNKNOWN(str) (str ? str : "unknown")
> + g_print("\n Phonebook entries count: %d\n",entry_count);
> + for (i=0; i< entry_count;i++)
> + {
The openening brace should be put in the same line as the for().
> + g_print("\tEntry index : %d \n"
> + "\t Number: %s \n"
> + "\t Name: %s \n",
> + phonebook_entries[i]->entry_index,
> + phonebook_entries[i]->number,
> + phonebook_entries[i]->name);
> + }
> +
> + mbim_phonebook_entry_array_free(phonebook_entries);
> + mbim_message_unref (response);
> + shutdown (TRUE);
> +}
> +
> +
Just 1 whiteline, no more.
> +static void
> +query_phonebook_configuration_ready (MbimDevice *device,
> + GAsyncResult *res)
> +{
> + MbimMessage *response;
> + GError *error = NULL;
> + MbimPhonebookState state;
> + const gchar *state_str;
> + guint32 number_of_entries;
> + guint32 used_entries;
> + guint32 max_number_length;
> + guint32 max_name;
> +
> + response = mbim_device_command_finish (device, res, &error);
> + if (!response) {
> + g_printerr ("error: operation failed: %s\n", error->message);
> + g_error_free (error);
> + shutdown (FALSE);
> + return;
> + }
> +
> + if(!mbim_message_phonebook_configuration_response_parse (
> + response,
> + &state,
> + &number_of_entries,
> + &used_entries,
> + &max_number_length,
> + &max_name,
> + &error)){
> + g_printerr ("error: couldn't parse response message: %s\n", error->message);
> + g_error_free (error);
> + shutdown (FALSE);
> + return;
> + }
> +
> + state_str = mbim_phonebook_state_get_string(state);
Always put a whitespace before the opening parenthesis; e.g.
"_get_string (state)".
> +#undef VALIDATE_UNKNOWN
> +#define VALIDATE_UNKNOWN(str) (str ? str : "unknown")
> +
> + g_print("\n Phonebook configuration retrived... \n"
> + "\t Phonebook state: %s \n"
> + "\t Number of entries: %d \n"
> + "\t used entries: %d \n"
> + "\t max number length: %d \n"
> + "\t max name : %d \n",
> + VALIDATE_UNKNOWN(state_str),
> + number_of_entries,
> + used_entries,
> + max_number_length,
> + max_name );
> +
Just 1 whiteline.
> +
> + mbim_message_unref (response);
> + shutdown (TRUE);
> +}
> +
Just 1 whiteline.
> +
> +void
> +mbimcli_phonebook_run (MbimDevice *device,
> + GCancellable *cancellable)
> +{
> + /* Initialize context */
> + ctx = g_slice_new (Context);
> + ctx->device = g_object_ref (device);
> + if (cancellable)
> + ctx->cancellable = g_object_ref (cancellable);
> +
> + /* Request to get configuration? */
> + if (phonebook_configuration_flag) {
> + MbimMessage *request;
> +
> + g_debug ("Asynchronously querying phonebook configurations...");
> + request = (mbim_message_phonebook_configuration_query_new (NULL));
Probably no need for the additional parenthesis around the method call.
> + mbim_device_command (ctx->device,
> + request,
> + 10,
> + ctx->cancellable,
> + (GAsyncReadyCallback)query_phonebook_configuration_ready,
> + NULL);
> + mbim_message_unref (request);
> + return;
> + }
> +
> + /*Phonebook read*/
Use a whitespace after '/*' and before '*/'. And apply this to all the
comments in the file as well.
> + if (phonebook_read_flag) {
> + MbimMessage *request;
> +
> + g_debug ("Asynchronously querying phonebook read...");
> + request = (mbim_message_phonebook_read_query_new (1,phonebook_index,NULL));
Always put a whiteline after each comma in the function call; e.g.
"(a, b, c)"; not "(a,b,c)".
Also, probably no need for the additional parenthesis around the method
call.
> + mbim_device_command (ctx->device,
> + request,
> + 10,
> + ctx->cancellable,
> + (GAsyncReadyCallback)query_phonebook_read_ready,
> + NULL);
> + mbim_message_unref (request);
> + return;
> + }
> +
> + /*Phonebook read*/
> + if (phonebook_read_all_flag) {
> + MbimMessage *request;
> +
> + g_debug ("Asynchronously querying phonebook read all...");
> + request = (mbim_message_phonebook_read_query_new (0,0,NULL));
Always put a whiteline after each comma in the function call; e.g.
"(a, b, c)"; not "(a,b,c)".
Also, probably no need for the additional parenthesis around the method
call.
> + mbim_device_command (ctx->device,
> + request,
> + 10,
> + ctx->cancellable,
> + (GAsyncReadyCallback)query_phonebook_read_ready,
> + NULL);
> + mbim_message_unref (request);
> + return;
> + }
Whiteline here.
> + /*Phonebook delete*/
> + if (phonebook_delete_flag) {
> + MbimMessage *request;
> +
> + g_debug ("Asynchronously phonebook delete...");
> + request = (mbim_message_phonebook_delete_set_new (1,phonebook_index,NULL));
Always put a whiteline after each comma in the function call; e.g.
"(a, b, c)"; not "(a,b,c)".
Also, probably no need for the additional parenthesis around the method
call.
> + mbim_device_command (ctx->device,
> + request,
> + 10,
> + ctx->cancellable,
> + (GAsyncReadyCallback)set_phonebook_delete_ready,
> + NULL);
> + mbim_message_unref (request);
> + return;
> + }
> +
> + /*Phonebook delete all*/
> + if (phonebook_delete_all_flag) {
> + MbimMessage *request;
> +
> + g_debug ("Asynchronously phonebook delete all...");
> + request = (mbim_message_phonebook_delete_set_new (0,0,NULL));
Always put a whiteline after each comma in the function call; e.g.
"(a, b, c)"; not "(a,b,c)".
Also, probably no need for the additional parenthesis around the method
call.
> + mbim_device_command (ctx->device,
> + request,
> + 10,
> + ctx->cancellable,
> + (GAsyncReadyCallback)set_phonebook_delete_ready,
> + NULL);
> + mbim_message_unref (request);
> + return;
> + }
Whiteline missing.
> + /*Phonebook write*/
Comment too indented.
> + if (phonebook_write_flag) {
> + MbimMessage *request;
> + g_debug ("Asynchronously writing phonebook...");
> + request = (mbim_message_phonebook_write_set_new (0,0,phonebook_number,phonebook_name,NULL));
Always put a whiteline after each comma in the function call; e.g.
"(a, b, c)"; not "(a,b,c)".
Also, probably no need for the additional parenthesis around the method
call.
> + mbim_device_command (ctx->device,
> + request,
> + 10,
> + ctx->cancellable,
> + (GAsyncReadyCallback)set_phonebook_write_ready,
> + NULL);
> + mbim_message_unref (request);
> + return;
> + }
> +
> +}
> diff --git a/cli/mbimcli.c b/cli/mbimcli.c
> index e4f9215..a892597 100644
> --- a/cli/mbimcli.c
> +++ b/cli/mbimcli.c
> @@ -254,6 +254,9 @@ device_open_ready (MbimDevice *dev,
> case MBIM_SERVICE_BASIC_CONNECT:
> mbimcli_basic_connect_run (dev, cancellable);
> return;
> + case MBIM_SERVICE_PHONEBOOK:
> + mbimcli_phonebook_run (dev, cancellable);
> + return;
> default:
> g_assert_not_reached ();
> }
> @@ -330,6 +333,10 @@ parse_actions (void)
> service = MBIM_SERVICE_BASIC_CONNECT;
> actions_enabled++;
> }
> + else if (mbimcli_phonebook_options_enabled ()) {
Add the "else if" line just after the closing brace in the previous line.
> + service = MBIM_SERVICE_PHONEBOOK;
> + actions_enabled++;
> + }
>
> /* Noop */
> if (noop_flag)
> @@ -364,6 +371,8 @@ int main (int argc, char **argv)
> context = g_option_context_new ("- Control MBIM devices");
> g_option_context_add_group (context,
> mbimcli_basic_connect_get_option_group ());
> + g_option_context_add_group (context,
> + mbimcli_phonebook_get_option_group ());
Need alignment in the previous line.
> g_option_context_add_main_entries (context, main_entries, NULL);
> if (!g_option_context_parse (context, &argc, &argv, &error)) {
> g_printerr ("error: %s\n",
> diff --git a/cli/mbimcli.h b/cli/mbimcli.h
> index 78942df..63a7afd 100644
> --- a/cli/mbimcli.h
> +++ b/cli/mbimcli.h
> @@ -28,9 +28,13 @@ void mbimcli_async_operation_done (gboolean operation_status);
>
> /* Basic Connect group */
> GOptionGroup *mbimcli_basic_connect_get_option_group (void);
> +GOptionGroup *mbimcli_phonebook_get_option_group (void);
> gboolean mbimcli_basic_connect_options_enabled (void);
> +gboolean mbimcli_phonebook_options_enabled (void);
Not a big deal, but align the "(void)" items with the ones in the lines
around.
> void mbimcli_basic_connect_run (MbimDevice *device,
> GCancellable *cancellable);
>
> +void mbimcli_phonebook_run (MbimDevice *device,
> + GCancellable *cancellable);
>
> #endif /* __MBIMCLI_H__ */
> -- 1.7.9.5
--
Aleksander
More information about the libmbim-devel
mailing list