Handle new EAP fields and provide backwards compatibility for old

Handle new EAP fields and provide backwards compatibility for old
CertPath users.

BUG=chromium-os:7101, chromium-os:10108
TEST=Connect to Google-A with existing chrome and entd/policy (w/ flimflam changes from 6132007).

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=c553c03

[Jason Glasgow, 2011-01-31 13:33:29]

A few comments.

http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc
File chromeos_network.cc (right):

http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode57
chromeos_network.cc:57: static const char* kCertPathProperty = "CertPath";
Do you want to mark this deprecated?  Only used for backward compatibility.

http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode117
chromeos_network.cc:117: static const char* kEAPClientCertProperty =
"EAP.ClientCert";
Is this the path to the cert?

http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode772
chromeos_network.cc:772: // Certificate path (backwards compat only)
Would it help to say // DEPRECATED so it is easy to find and remove later?

http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode1631
chromeos_network.cc:1631: continue; // TODO(njw): log something?
Not really a TODO, since the code is already DEPRECATED.

http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode1641
chromeos_network.cc:1641: ::g_hash_table_insert(properties,
::g_strdup(kCertPathProperty),
Is this not kEAPClientCertProperty?  Compare with line 1767.

http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode1718
chromeos_network.cc:1718: char **settings = ::g_strsplit_set(
Can you use scoped_ptr_malloc<> here?

http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode1740
chromeos_network.cc:1740: return false;
You will leak settings here unless you use the scoped_ptr_malloc<> above.

http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode1767
chromeos_network.cc:1767: kEAPClientCertProperty,
See line 1641.

[Nathan Williams, 2011-01-31 16:17:04]

On 2011/01/31 13:33:29, Jason Glasgow wrote:
> A few comments.
> 
> http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc
> File chromeos_network.cc (right):
> 
> http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode57
> chromeos_network.cc:57: static const char* kCertPathProperty = "CertPath";
> Do you want to mark this deprecated?  Only used for backward compatibility.
Sure, done.

> 
>
http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode117
> chromeos_network.cc:117: static const char* kEAPClientCertProperty =
> "EAP.ClientCert";
> Is this the path to the cert?
Or a link to a base64-encoded certificate blob, though we'd need a little extra
support for that. The name, like those of all of the other EAP.* properties, was
chosen to correspond to the name of the configuration variable in
wpa_supplicant.


>
http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode772
> chromeos_network.cc:772: // Certificate path (backwards compat only)
> Would it help to say // DEPRECATED so it is easy to find and remove later?
Sure, done.

 
>
http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode1631
> chromeos_network.cc:1631: continue; // TODO(njw): log something?
> Not really a TODO, since the code is already DEPRECATED.
Fixed by borrowing the code in the other block that logs about this.


>
http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode1641
> chromeos_network.cc:1641: ::g_hash_table_insert(properties,
> ::g_strdup(kCertPathProperty),
> Is this not kEAPClientCertProperty?  Compare with line 1767.
Oops, fixed.


>
http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode1718
> chromeos_network.cc:1718: char **settings = ::g_strsplit_set(
> Can you use scoped_ptr_malloc<> here?
> 
>
http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode1740
> chromeos_network.cc:1740: return false;
> You will leak settings here unless you use the scoped_ptr_malloc<> above.
Good catch. Fixed.


>
http://codereview.chromium.org/6293027/diff/2001/chromeos_network.cc#newcode1767
> chromeos_network.cc:1767: kEAPClientCertProperty,
> See line 1641.

PTAL.

[Jason Glasgow, 2011-01-31 16:29:01]

LGTM

[Nathan Williams, 2011-02-04 23:52:00]

On 2011/01/31 16:29:01, Jason Glasgow wrote:
> LGTM

I missed a spot (compat on the way back up as well as down). Please PTAL.

[Jason Glasgow, 2011-02-07 14:37:40]

LGTM

http://codereview.chromium.org/6293027/diff/13001/chromeos_network.cc
File chromeos_network.cc (right):

http://codereview.chromium.org/6293027/diff/13001/chromeos_network.cc#newcode779
chromeos_network.cc:779: certpath += StringPrintf("%scert_id=%s", comma ? "," :
"", info->cert_id);
Less code if char *comma=""; and then comma = ","
but either way LGTM

[Nathan Williams, 2011-02-07 14:50:19]

On 2011/02/07 14:37:40, Jason Glasgow wrote:
> LGTM
> 
> http://codereview.chromium.org/6293027/diff/13001/chromeos_network.cc
> File chromeos_network.cc (right):
> 
>
http://codereview.chromium.org/6293027/diff/13001/chromeos_network.cc#newcode779
> chromeos_network.cc:779: certpath += StringPrintf("%scert_id=%s", comma ? ","
:
> "", info->cert_id);
> Less code if char *comma=""; and then comma = ","
> but either way LGTM

Ah, that is cleaner. Done, thanks.