Re: [PATCH] Easy patch (help out!): preserve wireless-enabled and networking-enabled

[Tony Espy <espy canonical com>]

On 10/27/2009 10:29 AM, Jirka Klimes wrote:
> On Tuesday 27 October 2009 15:04:51 Tony Espy wrote:
>    
> > On 10/27/2009 07:58 AM, Jirka Klimes wrote:
> >      
> > > On Friday 23 October 2009 19:19:14 Dan Williams wrote:
> > >        
> > > > Anyone want to do a really simple patch?  It would close some bugs and
> > > > help out a lot:
> > > >          
> > > Hello Dan,
> > >
> > > I've done the patch. Please, review it.
> > >        
> > Jirka --
> >
> > You beat me to the punch...  I finished up my version last night, but
> > hadn't tested it yet.
> >
> >      
> Sorry for that. I hope you didn't spend much time on it.
> Nevertheless it's good when more people check the stuff.
>    

No worries...

> > You version is actually a little bit cleaner, as I hadn't created a new
> > struct for the config parameters.  I've attached my version ( which is
> > actually a quilt patch for Ubuntu ) for reference.
> >
> > The one thing I think you may have missed, is checking for
> > G_KEY_FILE_ERROR_INVALID_VALUE when reading the values from the file.
> > Other than that, looks good to me!
> >
> >      
> I'm aware of G_KEY_FILE_ERROR_INVALID_VALUE. However I decided not to produce
> errors for reading the values.
> So:
> missing keys -         regarded as TRUE
> erroneous values - regarded as FALSE
>    

Well, the only problem I see is that a bad value means you could end up 
with networking and/or wireless disabled.

That said, at least with your approach, a user can restore one or both 
via the applet, whereas if we return an error, the user has to figure 
out how to restart NetworkManger.

If we decide to stick with your approach, perhaps a log message is 
warranted?

One last thing that's been eating at me a little, is that we're saving 
program state to a configuration file.   As such, we could potentially 
lose state if we decide to drop a new configuration file into place in a 
future update.

[ Dan - any thoughts on creating a separate .state file instead? ]

Regards,
/tony