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

[Dan Williams <dcbw redhat com>]

On Tue, 2009-10-27 at 10:50 -0400, Tony Espy wrote:
> 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? ]

Yeah, that's probably valid.  I wonder where to put it though, since we
want this to be preserved across reboot, and /var/run or /var/lib isn't
guaranteed to be so, right?  That's usually what /etc is for.  Or are
you suggesting /etc/NetworkManager.state ?

Dan