Re: No error is returned to user if system settings plugin fails connection update

[Dan Williams <dcbw redhat com>]

On Sat, 2010-11-20 at 15:48 +0300, Andrey Borzenkov wrote:
> On Tue, Mar 2, 2010 at 8:34 PM, Andrey Borzenkov <arvidjaar gmail com> wrote:
> > On Tuesday 02 of March 2010 10:14:09 Dan Williams wrote:
> >> On Sat, 2010-02-27 at 23:39 +0300, Andrey Borzenkov wrote:
> >> > Using NM 0.8 + nm-applet 0.8 I hit the following situation - if
> >> > ->update plugin method fails (for whatever reason - e.g. plugin is
> >> > not able to create necessary file) - no error is returned to user.
> >> > Error is correctly returned if creation of new system connection
> >> > fails.
> >> >
> >> > I start nm-connection-editor and select system connection to
> >> > modify. I correctly get authentication request and am able to
> >> > modify connection (and save changes if no errors happened). But if
> >> > there are errors inside of plugin during updating of connection
> >> > settings, very funny situation results - logical state of NM is
> >> > updated (i.e. if I start n-c-e again, I see my changes) but state
> >> > in files on permanent storage is not updated, meaning restarting
> >> > NM show again previous configuration.
> >> >
> >> > I verified that plugin really returns error, error is correctly set
> >> > and plugin should be calling callback (in this case
> >> > con_update_cb). This unfortunately pretty much ends my ability to
> >> > debug it further - I am not good at D-Bus internals.
> >>
> >> So the flow *does* get to nm-sysconfig-connection.c's con_update_cb()
> >> function,
> >
> > Yes
> >
> >> and the error is valid in that function?
> >
> > Yes
> >
> >> If you add a
> >> g_message/fprintf of error->message there is that valid too?
> >>
> >
> > update: DEBUG: /etc/sysconfig/network-scripts/ifcfg-eth0 ERROR ifcfg-
> > mdv: DHCP_CLIENT_ID is not supported
> > con_update_cb: DEBUG: connection error ifcfg-mdv: DHCP_CLIENT_ID is not
> > supported
> >
> >
> 
> Sorry for reviving old thread, but it is still valid in 0.8.2. So far
> there are two separate issues associated with it.
> 
> 1) if plugin fails to write connection to stable store (for whatever
> reasons) NM is left with old, cached, connection that was received fro
> client. It makes impression that connection update worked (and it even
> will work for as long as either NM is restarted or connection is
> re-read again from disk). The reason is,
> src/system-settings/nm-sysconfig-connection.c:pk_update_cb() first
> updates in memory and then calls to save new definition. It is even
> documented in comments :)
> 
>         /* Update our settings internally so the update() call will save the new
>          * ones.
> 
> But if ->update fails, settings are not rolled back.
> 
> It could be possible; we could add old_connection field to PolkitCall
> and use it to rollback in con_update_cb. Any potential issues
> associated with it?

Yeah, we could do that, and hopefully return an error to the client that
called update() in the first place.

> 2) NM core does send proper error via D-Bus to nm-connection-editor,
> but it simply does not have code to display them. On update it finally
> calls connection_updated_cb() and it does not display anything (nor is
> any error displayed on callback chain).

Yeah, we should do better here.  I thought we had an error dialog that
popped up for stuff like this?  The code there is somewhat convoluted,
but ideally we'd notify the user that an error occurred.

Dan