Open Source Telephony
 help / color / mirror / Atom feed
From: Marcel Holtmann <marcel@holtmann.org>
To: ofono@ofono.org
Subject: Re: [patch 6/6] Allow gsmdial to use gatchat ppp support
Date: Tue, 16 Mar 2010 23:42:35 -0700	[thread overview]
Message-ID: <1268808155.2700.84.camel@localhost.localdomain> (raw)
In-Reply-To: <20100316171330.5fea7878@kcaccard-MOBL3>

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

Hi Kristen,
 
>  static GAtChat *control;
>  static GAtChat *modem;
>  static GMainLoop *event_loop;
> +static GAtPPP *ppp;

I hate myself for this nitpick, but please group GAtPPP *ppp with the
others before the mainloop variable.
 
> +static void print_ip_address(guint32 ip_addr)
> +{
> +	struct in_addr addr;
> +	addr.s_addr = ip_addr;
> +	g_print("%s\n", inet_ntoa(addr));
> +}

Please use print_ip_address(const char *label, guint32 ip_addr) as
mentioned in the previous review.

> +	/* open ppp */
> +	ppp = g_at_ppp_new(channel);
> +	if (!ppp) {
> +		g_print("Unable to obtain PPP object\n");
> +		return;
> +	}

I would call it "Unable to create PPP object", because you are creating.
My understanding is the obtaining means that it would be there in the
first place. It is a nitpick.

Regards

Marcel



  reply	other threads:[~2010-03-17  6:42 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20100317000824.420232401@linux.intel.com>
2010-03-17  0:13 ` [patch 1/6] Add PPP protocol support with HDLC framing Kristen Carlson Accardi
2010-03-17  6:28   ` Marcel Holtmann
2010-03-17  0:13 ` [patch 2/6] Generic PPP control protocol Kristen Carlson Accardi
2010-03-17  7:06   ` Marcel Holtmann
2010-03-18 23:03     ` Kristen Carlson Accardi
2010-03-19  3:57       ` Marcel Holtmann
2010-03-17  0:13 ` [patch 3/6] LCP support Kristen Carlson Accardi
2010-03-17  7:09   ` Marcel Holtmann
2010-03-17 18:59     ` Kristen Carlson Accardi
2010-03-17 19:31       ` Marcel Holtmann
2010-03-17  0:13 ` [patch 4/6] CHAP with MD5 authentication Kristen Carlson Accardi
2010-03-17  7:12   ` Marcel Holtmann
2010-03-17  0:13 ` [patch 5/6] IP support for PPP Kristen Carlson Accardi
2010-03-17  6:38   ` Marcel Holtmann
2010-03-18  0:12     ` Kristen Carlson Accardi
2010-03-18 11:26       ` Marcel Holtmann
2010-03-19 17:58         ` Kristen Carlson Accardi
2010-03-17  0:13 ` [patch 6/6] Allow gsmdial to use gatchat ppp support Kristen Carlson Accardi
2010-03-17  6:42   ` Marcel Holtmann [this message]
     [not found] <20100319194705.214150215@linux.intel.com>
2010-03-19 19:46 ` Kristen Carlson Accardi
2010-03-20 15:35   ` Marcel Holtmann
     [not found] <20100311214022.838696145@linux.intel.com>
2010-03-11 22:00 ` kristen
2010-03-14 20:02   ` Marcel Holtmann
2010-03-16 23:52     ` Kristen Carlson Accardi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1268808155.2700.84.camel@localhost.localdomain \
    --to=marcel@holtmann.org \
    --cc=ofono@ofono.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox