GNOME Bugzilla – Bug 735824
NetworkManager-0.9.10.0 complains about . files in the system-connections directory
Last modified: 2015-02-23 13:47:52 UTC
<warn> error in connection /etc/NetworkManager/system-connections/.keep_net-apps_NetworkManager-0: invalid or missing connection property '(null)/connection setting not found' Not sure how you think about this since it's probably distro-specific, we install these .keep files on Exherbo to let our package manager install empty directories. Is it intended that NetworkManager also reads dot files and if not could you exclude them from being read?
I agree that NetworkManager should ignore dot files. It will be useful for many distributions and one could also (mis)use it to hide a connection file from NetworkManager but still keep it around.
Created attachment 285131 [details] [review] patch to fix the issue
NetworkManager-0.9.10.0 with the patch applied fails to compile for me with the following error, complete log attached. utils.c:24:33: fatal error: gsystem-local-alloc.h: No such file or directory #include "gsystem-local-alloc.h" ^ compilation terminated. Makefile:619: recipe for target 'utils.lo' failed make[6]: *** [utils.lo] Error 1
Created attachment 285135 [details] NetworkManager-0.9.10.0-patched.log
NetworkManager-0.9.10.0 compiles for me when changing #include "gsystem-local-alloc.h" to #include "libgsystem/gsystem-local-alloc.h" in the provided patch.
So you have your patch to be used in a distribution, and I propose the attached patch to be commmited to upstream git. Plus it would be nice to propagate the changes for libgsystem and this change to nm-0-9-10.
One drawback now is that previously, if you *did* name a connection ".my-connection" it would get written as such, and would now be ignored. I do think this is a low-risk issue though. _writer_id_to_filename() doesn't have great protection against odd characters in the file id, and we should fix that at the same so that you cannot write a connection which would then immediately be ignored by the plugin. I don't have a problem with the above patch, but we need an additional section to fix up _writer_id_to_filename() for those cases too.
Created attachment 294522 [details] [review] keyfile: use locale independent g_ascii_strcasecmp()
Created attachment 294523 [details] [review] keyfile: add nm_keyfile_plugin_utils_escape_filename() function We have nm_keyfile_plugin_utils_should_ignore_file() to ignore certain files based on patterns. We also need a matching escape function to avoid saving connections with a name we would ignore later.
Created attachment 294524 [details] [review] keyfile: ignore all dot files (bgo#735824)
Comment on attachment 294524 [details] [review] keyfile: ignore all dot files (bgo#735824) reposted in attachment 294524 [details] [review]
Comment on attachment 294524 [details] [review] keyfile: ignore all dot files (bgo#735824) (obsoleted wrong attachment :( Undo )
Comment on attachment 285131 [details] [review] patch to fix the issue reposted in attachment 294524 [details] [review]
Review of attachment 294522 [details] [review]: LGTM
Review of attachment 294523 [details] [review]: ::: src/settings/plugins/keyfile/utils.c @@ +128,3 @@ + g_string_append_c (str, ESCAPE_CHAR); + break; + default: At this point, there's only one escape char, so I would say simply make the switch into an if/else. @@ +146,3 @@ + + return g_string_free (str, FALSE);; +} Also, lets get a testcase for this function to make sure that the code doc about should_ignore_file() mirroring this function is true? ::: src/settings/plugins/keyfile/writer.c @@ +844,3 @@ + + filename_escaped = nm_keyfile_plugin_utils_escape_filename (filename); + Hmm, what if the escaped name also clashes with another connection? Since ID != filename, you could have two connections like: ///foobar~ ***foobar* and both would receive the same escaped filename. Do we care about this?
Review of attachment 294524 [details] [review]: ::: src/settings/plugins/keyfile/utils.c @@ +106,3 @@ + if (check_suffix (base, PEM_TAG) || check_suffix (base, DER_TAG)) + return TRUE; + Can we get rid of SWP_TAG and SWPX_TAG now? I don't think they're used anymore...
I addressed the comments and pushed the 3 patches to branch th/keyfile-escape-file-bgo735824. (In reply to comment #15) > Review of attachment 294523 [details] [review]: > > ::: src/settings/plugins/keyfile/utils.c > @@ +128,3 @@ > + g_string_append_c (str, ESCAPE_CHAR); > + break; > + default: > > At this point, there's only one escape char, so I would say simply make the > switch into an if/else. done > @@ +146,3 @@ > + > + return g_string_free (str, FALSE);; > +} > > Also, lets get a testcase for this function to make sure that the code doc > about should_ignore_file() mirroring this function is true? done > ::: src/settings/plugins/keyfile/writer.c > @@ +844,3 @@ > + > + filename_escaped = nm_keyfile_plugin_utils_escape_filename > (filename); > + > > Hmm, what if the escaped name also clashes with another connection? Since ID > != filename, you could have two connections like: > > ///foobar~ > ***foobar* > > and both would receive the same escaped filename. Do we care about this? we care about it, but as the code in _internal_write_connection() is, this is no problem, because we would try another filename. (In reply to comment #16) > Review of attachment 294524 [details] [review]: > > ::: src/settings/plugins/keyfile/utils.c > @@ +106,3 @@ > + if (check_suffix (base, PEM_TAG) || check_suffix (base, DER_TAG)) > + return TRUE; > + > > Can we get rid of SWP_TAG and SWPX_TAG now? I don't think they're used > anymore... I would let it there. This was added by commit c1dd530798301be08197a390516a44c423bfc029, guess there was a reason for it? Some old system-connections/ directories might contains such files. I feel changing this behavior gives not benefit. Once the escape function matches the ignore function, we can ignore more then strictly necessary.
merged to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=6ccb88883103884459afb17b51748a1cf8b648fd