Re: __attribute__ ((cleanup) patch
From: Colin Walters walt...@verbum.org So the patch ended up getting reverted, which I'm OK with, Thank you for understanding. I'll also try to reply so that we have a clean technical discussion, separate from my ranting :). but what I still want to know is - what are the hurdles I need to jump to move this forward? One thing I would like to give it some reasonable time. I'd still like to consult some people. Pavel mentioned he felt like NetworkManager was being used as an experimental testbed; I don't quite agree with that characterization, but for example, is having more projects use the local allocation something that would help? The number of other projects is not my primary concern. Rather it is the value and the risk it brings to the project, and especially risk to the networking community perception of this change. From my point of view, the value is rather small. That's not bad, I appreciate small steps towards the right direction. But I definitely want to review carefully rather small improvements that are potentially problematic. I can work on that. I'd actually hoped to do NM first since I still plan to do some development on it, but that's OK. Understood. More documentation on how to use it? Yes. Document which compilers support it? Yes, and which of the major compilers don't (their reason would be nice). Detect if we don't have it in configure.ac and error out? Yeah, that would prevent unpleasant surprise. Other stuff? Apart from time, information and coordination with other changes, I can't think about anything. Maybe others? A couple of programmers questions, though, that will help me to assess the actual value of gsystem: 1) I want to create a variable of object pointer type to be NULL. I want it freed *if and only if* I assign it a value (an actual allocated object). Is that possible? Are you going to use it? 2) How does this work with tools like Valgrind. 3) What about some static analysis tools? 4) Any known issues with GDB? 5) What does it do on exit(), abort() and fatal signals? 6) Any other things you can think about? Some documentation distributed with the source code? FAQ with stupid questions like those above? These things are extremely helpful. Pavel ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
On Fri, 2012-10-26 at 09:54 -0400, Pavel Simerda wrote: One thing I would like to give it some reasonable time. I'd still like to consult some people. Please do forward any feedback. A couple of programmers questions, though, that will help me to assess the actual value of gsystem: I'll create a wiki page soon and answer some of this stuff there. ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
On Thu, 2012-10-18 at 11:59 -0500, Dan Williams wrote: On Thu, 2012-10-18 at 11:56 -0400, Colin Walters wrote: On Thu, 2012-10-18 at 11:51 -0400, Colin Walters wrote: I don't oppose that. Attached. Well, *if* we did that, we'd just git revert sha instead of applying a patch to do it. So the patch ended up getting reverted, which I'm OK with, but what I still want to know is - what are the hurdles I need to jump to move this forward? Pavel mentioned he felt like NetworkManager was being used as an experimental testbed; I don't quite agree with that characterization, but for example, is having more projects use the local allocation something that would help? I can work on that. I'd actually hoped to do NM first since I still plan to do some development on it, but that's OK. More documentation on how to use it? Document which compilers support it? Detect if we don't have it in configure.ac and error out? Other stuff? ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
On Fri, 2012-10-19 at 12:05 -0400, Pavel Simerda wrote: I, personally, was considering using some faster compiler for development. Pavel Tišnovský wrote an article about tcc and it might save me a lot of time waiting for compilation. I doubt it, though it does depend on what kind of build you're doing. For developer incremental builds (i.e. you haven't touched the Makefiles, only modified .[ch] files), where you have a decently fast machine (circa 2010+ CPU and hard disk), and a warm cache from a recent build...you're talking about a ballpark possibility of going from 1.5 seconds to 1.3 I'd guess. Just not worth it. Also, from a quick inspection, tcc isn't going to speed up g-ir-compiler, unfortunately one of the slower parts of the build (my fault actually, but very hard to make faster). And even if it's faster, how good is the DWARF generation? Your build may be even 10 seconds faster, but if you have to rebuild it when you want to use GDB, that's not really a win. Anyways if you come back to me and tell me that building with tcc is useful for you, I bet I can gain back a substantial amount of the lost speed (or more) by e.g. switching to nonrecursive automake. ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
From: Colin Walters walt...@verbum.org On Fri, 2012-10-19 at 12:05 -0400, Pavel Simerda wrote: I, personally, was considering using some faster compiler for development. Pavel Tišnovský wrote an article about tcc and it might save me a lot of time waiting for compilation. I doubt it, though it does depend on what kind of build you're doing. For developer incremental builds (i.e. you haven't touched the Makefiles, only modified .[ch] files), Moving changes around, often. Rebasing, bisecting, testing if a multitude of subsequent commits actually build. where you have a decently fast machine (circa 2010+ CPU and hard disk), and a warm cache from a recent build...you're talking about a ballpark possibility of going from 1.5 seconds to 1.3 I'd guess. Just not worth it. Once I get down to 1.5 seconds, we can talk about it :D. I just realised I'm *not* using ccache so let's see how much it helps. Also, from a quick inspection, tcc isn't going to speed up g-ir-compiler, unfortunately one of the slower parts of the build (my fault actually, but very hard to make faster). That's another question, yes. And even if it's faster, how good is the DWARF generation? Your build may be even 10 seconds faster, but if you have to rebuild it when you want to use GDB, that's not really a win. I don't have enough information for that one. Anyways if you come back to me and tell me that building with tcc is useful for you, I bet I can gain back a substantial amount of the lost speed (or more) by e.g. switching to nonrecursive automake. Could be of interest. Or maybe just making the recursion less aggressive. I am going to do some configure.ac cleanups anyway as its current state is horrible. But I wasn't going to do much with Makefile.am's. Btw, I had to see myself, so I tested LLVM's support for __attribute__((cleanup)) with success. Pavel ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
We have been too conservative to adopt the C99 standard and now we are brave enough to go for non-standard compiler features. We already use some C99 features; basic stuff like __func__ and snprintf(). I don't have a problem with using -std=c99 to build stuff, but it seems like either GCC is already exposing those features with -std=gnu89. The one thing I *don't* like in c99 is the mixed declarations and code; that clutters up the code horribly. This is a matter of opinion. Creating a variable when I need it may be sometimes much more readable. And, without that, you can't use variable sized automatic arrays which are one of the things Colin's patch is good for (although g_alloca works well here, too). NetworkManager is tied to Linux, now, but in my opinion not as highly as it might seem at the first glance. Dan might disagree. It is not a goal of NetworkManager to actively support other platforms. We're not talking about active support here, but about potentially actively blocking this. And I'm the last one to actively oppose the BSD or *any other* community from working with us. If there was a nice, easy way to support *BSD (and with the platform stuff maybe there would be) then we'd entertain the patch, but we have enough things to work on at the moment than BSD support. Give me an answer to the following questions. 1) Do we consider the Linux non-GCC community if *they* want to use NetworkManager? 2) Do we consider the BSD community if *they* want to port NetworkManager? 3) Do we consider the possibility that someone else, non-Linux, non-BSD would like to work join the NetworkManager effort, use it and help us with the development? I think that regardless the actual technical aspects, by doing what we do here, we say: “We are NetworkManager. We are Linux. We are GCC. Otherwise, fuck off.” On the other hand, reverting, and starting again in a community-positive way, we would be saying: “Yes, you are welcome to work with us and we are ready to listen to your concerns and *then* decide.” I'll be meeting with SUSE networking developers during the weekend. What should I tell them? That it's not worth trying to work with us unless they are ready to see big changes going under their hands? it's not like we have to care about MSVC. Nobody's talking about MSVC here. Now, I plan to do a followup patch pretty soon which would let us get rid of a *lot* of g_free() You can get rid of g_free's on small strings for free with a pretty standard way through g_alloca, for example. For that, this is overkill. That doesn't work for allocated values passed back from other functions, which happens quite a bit. But the libgsystem stuff does work for that AFAIK. ie: I'm sorry but I was reacting solely to the patch as there was no public discussion. Plus I did some quick googling, but you shouldn't expect that. And that's me, an active developer. The community needs more than that to accomodate the changes. And if we do rude things like that, why didn't we drop libnl1/2. This might have saved more work. Yes, and other dependencies. I believe that this is a matter of attitude. Either we prove to be assholes who don't care, or we maintain a healthy open source project. It's not possible to do both. char *foo; call_some_func (foo); do_something g_free (foo); cannot be made simpler with g_alloca(). True. /g_object_unref() calls. For this, I would need some more detailed information. This is just not enough for merging someone's experiments that don't even have any documentation. I admit it is tempting to use a real important project for one's experiments but it might not be the right direction for the project. I don't consider this an experiment at this point. That surprises me. We're unlikely to convert to C++ any time soon, and anything that makes C easier to code, reduces programmer errors and memory leaks, and cleans up the existing code is a win, as long as it's not too complicated. Given that this stuff is *less* complicated than the existing code, it's a win. Yes, there is a bit of education required, but perhaps we could also work on making some things compile-time errors instead of runtime gotchas. I'm not comfortable seeing these changes made by force (git push to master). What I would expect, is to have a public discussion, with arguments for, arguments against, and an educated decision afterwards. conservative estimate, around 1500 lines. I've been using libgsystem cleanup extensively in several projects, and it's been a massive win. Then why it wasn't good enough for Glib but is good enough for NetworkManager? Why I don't see a link to elaborate explanation and documentation? Success stories. More information? Other examples of projects using this in C are systemd and upstart. The downside is that this is kind of a one-way door. Once done, it'd
Re: __attribute__ ((cleanup) patch
I didn't have time to study it and make an opinion. And I'm preparing for the LinuxDays conference, so I can hardly do that now. There may be others who need more than just a few days. So my current opinion is that it *might* be helpful and it *might* be a big problem. And there are others, more proficient in some subjects, who I would like to confront with this idea before proceeding. Another thing is, that you are going to make changes across the whole tree which should not be done without coordination. Pavel - Original Message - From: Colin Walters walt...@verbum.org To: Pavel Simerda psime...@redhat.com Cc: networkmanager-list@gnome.org Sent: Thursday, October 18, 2012 7:19:13 PM Subject: Re: __attribute__ ((cleanup) patch On Thu, 2012-10-18 at 11:19 -0400, Pavel Simerda wrote: I'm not yet even convinced about this because of total lack of documentation to be found right away. What do you think about this patch? Does it help address your concerns? Is there anything that could be clearer? ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
On Fri, 2012-10-19 at 08:47 -0400, Pavel Simerda wrote: 1) Do we consider the Linux non-GCC community if *they* want to use NetworkManager? So concretely, the major ones are LLVM and ICC; both of these I know both implement a lot of the GCC extensions, and they're also C++ compilers, which means implementing this feature should be trivial. (I just double checked; clang handles __attribute__ ((cleanup)) just fine) The other ones like PCC - yeah, just ignore it. Basically if you look at how much __attribute__ ((cleanup)) improves the code in one hand, and what PCC brings (basically nothing over GCC/LLVM) in the other hand... I think that regardless the actual technical aspects, by doing what we do here, we say: “We are NetworkManager. We are Linux. We are GCC. Otherwise, fuck off.” I don't agree the change says that. A lot of the useful bits of GNU C have become a de facto C language subset that are implemented by the important C compilers (with the major exception of MSVC). So again, this isn't restricted to just GCC. On the other hand, reverting, and starting again in a community-positive way, we would be saying: “Yes, you are welcome to work with us and we are ready to listen to your concerns and *then* decide.” I'll be meeting with SUSE networking developers during the weekend. What should I tell them? That it's not worth trying to work with us unless they are ready to see big changes going under their hands? I believe that this is a matter of attitude. Either we prove to be assholes who don't care, or we maintain a healthy open source project. It's not possible to do both. The technical debates about compilers though do pale beside this issue, and again, let me apologize. *I* screwed up. You are absolutely right that this should have gone through the mailing list. (It was public in Bugzilla, but I didn't really elaborate on it much there). I guess to show that we know we screwed up is not good enough. Talking about a change that has been merge is ranting. I think your concerns are reasonable, and it's not ranting. Post-commit debate happens in larger projects too; look at linux-kernel. And some of that has definitely resulted in commits being reverted. Other times, the author of the commit follows up and addresses the concerns. I'm not going to pretend a discussion over something that has been already decided. Ultimately I guess as project maintainer it's dcbw's call, but that I still think this discussion is valuable. But what I need is a list of problems to address, and feedback about how I'm addressing them. Documentation is one, and if you got a chance to review that patch, it'd be appreciated. The other concern is about process, and all I can say there is I apologize, and it won't happen again. I'm off to hopefully get some more projects like gdm to use this, and I'll make sure all the maintainers and consumers are in the loop. ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
From: Colin Walters walt...@verbum.org On Fri, 2012-10-19 at 08:47 -0400, Pavel Simerda wrote: 1) Do we consider the Linux non-GCC community if *they* want to use NetworkManager? So concretely, the major ones are LLVM and ICC; both of these I know both implement a lot of the GCC extensions, and they're also C++ compilers, which means implementing this feature should be trivial. I, personally, was considering using some faster compiler for development. Pavel Tišnovský wrote an article about tcc and it might save me a lot of time waiting for compilation. (I just double checked; clang handles __attribute__ ((cleanup)) just fine) That's a good news. The other ones like PCC - yeah, just ignore it. Basically if you look at how much __attribute__ ((cleanup)) improves the code in one hand, and what PCC brings (basically nothing over GCC/LLVM) in the other hand... I think that regardless the actual technical aspects, by doing what we do here, we say: “We are NetworkManager. We are Linux. We are GCC. Otherwise, fuck off.” I don't agree the change says that. By a large scale change in the master code base that may affect users and developers for a decade or more, without any communication, this is exactly what *we* do. This has *nothing* to do with your particular change, but rather it is related to the way changes like this are propagated. A lot of the useful bits of GNU C have become a de facto C language subset that are implemented by the important C compilers (with the major exception of MSVC). So again, this isn't restricted to just GCC. The lack of information about various compiler projects that might be used to compile NetworkManager during the nearest decade, is a problem in itself. You may have enough information, but many other developers, packagers and advanced users don't. On the other hand, reverting, and starting again in a community-positive way, we would be saying: “Yes, you are welcome to work with us and we are ready to listen to your concerns and *then* decide.” I'll be meeting with SUSE networking developers during the weekend. What should I tell them? That it's not worth trying to work with us unless they are ready to see big changes going under their hands? I believe that this is a matter of attitude. Either we prove to be assholes who don't care, or we maintain a healthy open source project. It's not possible to do both. The technical debates about compilers though do pale beside this issue, and again, let me apologize. I value your cooperation. *I* screwed up. I say *we* because I feel we are trying to work together. You might have gone through the mailing list. Dan might have asked you to do so. I might have explicitly expressed my ideas about using another compiler earlier and anyone else might have done this as well. And *we all* failed in setting up some common understanding of what changes should be discussed and how much. My opinion on many things is different from other opinions. You are absolutely right that this should have gone through the mailing list. (It was public in Bugzilla, but I didn't really elaborate on it much there). Actually, it might even help us to handle similar future cases better. And that's what I'm trying to achieve. I guess to show that we know we screwed up is not good enough. Talking about a change that has been merge is ranting. I think your concerns are reasonable, and it's not ranting. Post-commit debate happens in larger projects too; look at linux-kernel. And some of that has definitely resulted in commits being reverted. Other times, the author of the commit follows up and addresses the concerns. I'm not going to pretend a discussion over something that has been already decided. Ultimately I guess as project maintainer it's dcbw's call, but that I still think this discussion is valuable. But what I need is a list of problems to address, and feedback about how I'm addressing them. Documentation is one, and if you got a chance to review that patch, it'd be appreciated. Ok. That said... I would like to know which compilers will be out of play and which are currently supported. Mostly from others, I would like to know which of those compilers might be a problem for people wanted to port NetworkManager to another Linux or non-Linux system. Keeping in mind both desktop and embedded usecases. While systemd folks explicitly discourage other platforms, as far as I know, we have not done that yet. The other concern is about process, and all I can say there is I apologize, and it won't happen again. And I would be happy if you're not the only person to agree, because this may happen to anyone working on an improvement in good faith. I'm off to hopefully get some more projects like gdm to use this, and I'll make sure all the maintainers and consumers are in the loop. It might be nice to have some common discussion and to
Re: __attribute__ ((cleanup) patch
From: Colin Walters walt...@verbum.org Hi, Hi, thanks for a quick reply. https://bugzilla.gnome.org/show_bug.cgi?id=685440 has a patch which just landed, but I wanted to give wider discussion to this, because it's a very important infrastructural change. First, one thing that came up is a concern about a GCC hard dependency. Yes. My understanding is that LLVM implements this too. We have been too conservative to adopt the C99 standard and now we are brave enough to go for non-standard compiler features. For compilers which implement C++, this is easy to support. I don't deny this, but this is not how the question stands, currently. And since NetworkManager is highly tied to Linux NetworkManager is tied to Linux, now, but in my opinion not as highly as it might seem at the first glance. Dan might disagree. it's not like we have to care about MSVC. Nobody's talking about MSVC here. Now, I plan to do a followup patch pretty soon which would let us get rid of a *lot* of g_free() You can get rid of g_free's on small strings for free with a pretty standard way through g_alloca, for example. For that, this is overkill. /g_object_unref() calls. For this, I would need some more detailed information. This is just not enough for merging someone's experiments that don't even have any documentation. I admit it is tempting to use a real important project for one's experiments but it might not be the right direction for the project. conservative estimate, around 1500 lines. I've been using libgsystem cleanup extensively in several projects, and it's been a massive win. Then why it wasn't good enough for Glib but is good enough for NetworkManager? Why I don't see a link to elaborate explanation and documentation? Success stories. More information? Other examples of projects using this in C are systemd and upstart. The downside is that this is kind of a one-way door. Once done, it'd be immensely tedious to try to go back and add them again. That's exactly the problem I have with such quick inclusions without a proper discussion. However, the benefit to the code is huge, I'm not yet even convinced about this because of total lack of documentation to be found right away. And I'm not convinced that switching to a non-standard programming language is the price to pay. and even better - trust me, it makes writing C fun again =) You mean writing a language incompatible with the C standards :). Changing the programming language for the whole project in a forward-only compatible manner is a big decision and I'm not really happy with the way it has been done. So, I haven't changed my mind. I propose the following path: 1) Revert 2) Document including the reasons why this cannot be added to Glib but should be added to NetworkManager 3) Discuss 4) Decide I don't think this should be a one-week process, so I personally insist on starting with the step (1). I would like to discourage this practice for future decision making regarding such huge one-way changes. Cheers, Pavel ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
On Thu, 2012-10-18 at 11:19 -0400, Pavel Simerda wrote: Then why it wasn't good enough for Glib but is good enough for NetworkManager? Why See this thread: https://mail.gnome.org/archives/gtk-devel-list/2012-April/msg3.html Basically, because the GTK+ stack needs to compile with MSVC. Unfortunately. And Microsoft said they'll never implement anything beyond C89, not even C99. I don't see a link to elaborate explanation and documentation? I'll add some more docs to libgsystem. But the most important thing to read first is http://gcc.gnu.org/onlinedocs/gcc/Variable-Attributes.html Success stories. Like I said, systemd and upstart both use it, and I completely rely on it inside ostree (http://git.gnome.org/browse/ostree) which is 15,000 lines of GObject/C. A tenth the size of NM, true, but also not a trivial project. We're using it in another currently Red Hat internal project. I think Ray is amenable to doing it for GDM. Ultimately though, the only way the library is going to get better is if people use it and help out. More information? I did blog a bit ago about it too: http://blog.verbum.org/2012/05/09/__attribute__-cleanup-or-how-i-came-to-love-c-again/ That's exactly the problem I have with such quick inclusions without a proper discussion. Fair enough. I'm not yet even convinced about this because of total lack of documentation to be found right away. Ok, I'll add some more. And I'm not convinced that switching to a non-standard programming language is the price to pay. It's kind of a leap to call this a non-standard programming language. In reality of course, GLib (which NM relies on) is a *heavy* user of GCC extensions. Things like atomic intrinstics and statement expressions. This is larger, too, but it's really just C with one feature from C++. So, I haven't changed my mind. I propose the following path: 1) Revert I don't oppose that. 2) Document including the reasons why this cannot be added to Glib but should be added to NetworkManager Already done here. 3) Discuss In progress =) ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
On Thu, 2012-10-18 at 10:19 -0400, Colin Walters wrote: Hi, https://bugzilla.gnome.org/show_bug.cgi?id=685440 has a patch which just landed, but I wanted to give wider discussion to this, because it's a very important infrastructural change. First, one thing that came up is a concern about a GCC hard dependency. My understanding is that LLVM implements this too. For compilers which implement C++, this is easy to support. And since NetworkManager is highly tied to Linux, it's not like we have to care about MSVC. LLVM is interesting, but until it (a) gets better adoption for major projects, and (b) can compile NetworkManager with very minimal changes to the NM codebase (ie, few if any #ifdefs), LLVM support is not a major goal of NetworkManager. It may be a goal in the future; and thus it would be nice to know if LLVM builds fine with these extensions. Now, I plan to do a followup patch pretty soon which would let us get rid of a *lot* of g_free()/g_object_unref() calls. By a conservative estimate, around 1500 lines. Yeah; I write that code all the time and every time I do, I wish I could use the attribute stuff. I've been using libgsystem cleanup extensively in several projects, and it's been a massive win. Other examples of projects using this in C are systemd and upstart. The downside is that this is kind of a one-way door. Once done, it'd be immensely tedious to try to go back and add them again. However, the benefit to the code is huge, and even better - trust me, it makes writing C fun again =) Yes, that's the major downside. The other potential issues are: 1) developer errors by choosing the wrong variable type; unless they are somehow type-safe. This is more of an education problem than anything else 2) educating everyone who touches the code about the new method Personally, I consider those as fixable issues, and outweighed by the benefit of having the variables automatically freed, which reduces memory leaks in NetworkManager, and makes the code more readable. Dan ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
On Thu, 2012-10-18 at 11:51 -0400, Colin Walters wrote:
I don't oppose that.
Attached.
From c43e095419423b36544e221f9f0896d2579fb0a0 Mon Sep 17 00:00:00 2001
From: Colin Walters walt...@verbum.org
Date: Thu, 18 Oct 2012 11:53:05 -0400
Subject: [PATCH] Revert core: import libgsystem, use it for
local-allocations in main.c (bgo #685440)
This reverts commit 89623b99c44bd7c6b721955ebc0a9550cb8dd978 pending
further discussion.
See: https://mail.gnome.org/archives/networkmanager-list/2012-October/msg00065.html
---
src/Makefile.am | 14 +
src/libgsystem/Makefile-libgsystem.am | 31 ---
src/libgsystem/README | 7 ---
src/libgsystem/README-NetworkManager-import | 5 --
src/libgsystem/gsystem-file-utils.c | 86 -
src/libgsystem/gsystem-file-utils.h | 34
src/libgsystem/gsystem-local-alloc.c| 65 --
src/libgsystem/gsystem-local-alloc.h| 42 --
src/libgsystem/libgsystem.doap | 31 ---
src/libgsystem/libgsystem.h | 33 ---
src/main.c | 76 +
11 files changed, 56 insertions(+), 368 deletions(-)
delete mode 100644 src/libgsystem/Makefile-libgsystem.am
delete mode 100644 src/libgsystem/README
delete mode 100644 src/libgsystem/README-NetworkManager-import
delete mode 100644 src/libgsystem/gsystem-file-utils.c
delete mode 100644 src/libgsystem/gsystem-file-utils.h
delete mode 100644 src/libgsystem/gsystem-local-alloc.c
delete mode 100644 src/libgsystem/gsystem-local-alloc.h
delete mode 100644 src/libgsystem/libgsystem.doap
delete mode 100644 src/libgsystem/libgsystem.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 697203a..ba7d2d6 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -21,20 +21,11 @@ endif
SUBDIRS += . tests
-noinst_LTLIBRARIES =
-EXTRA_DIST =
-NULL =
-libgsystem_srcpath := libgsystem
-libgsystem_cflags = $(GIO_CFLAGS)
-libgsystem_libs = $(GIO_LIBS)
-include libgsystem/Makefile-libgsystem.am
-
INCLUDES = -I${top_srcdir} \
-I${top_builddir}/include \
-I${top_srcdir}/include \
-I${top_builddir}/src/generated \
-I${top_srcdir}/src/generated \
- -I${top_srcdir}/src/libgsystem \
-I${top_srcdir}/src/logging \
-I${top_srcdir}/src/posix-signals \
-I${top_srcdir}/src/dns-manager \
@@ -56,7 +47,7 @@ INCLUDES = -I${top_srcdir} \
# Test libraries
###
-noinst_LTLIBRARIES += \
+noinst_LTLIBRARIES = \
libtest-dhcp.la \
libtest-policy-hosts.la \
libtest-wifi-ap-utils.la
@@ -322,7 +313,6 @@ endif
NetworkManager_LDADD = \
./generated/libnm-generated.la \
- libgsystem.la \
./logging/libnm-logging.la \
./posix-signals/libnm-posix-signals.la \
./dns-manager/libdns-manager.la \
@@ -373,7 +363,7 @@ NetworkManager_DATA = gdb-cmd
dbusservicedir = $(DBUS_SYS_DIR)
dbusservice_DATA = org.freedesktop.NetworkManager.conf
-EXTRA_DIST += \
+EXTRA_DIST = \
$(dbusservice_DATA) \
$(NetworkManager_DATA)
diff --git a/src/libgsystem/Makefile-libgsystem.am b/src/libgsystem/Makefile-libgsystem.am
deleted file mode 100644
index b0e87c5..000
--- a/src/libgsystem/Makefile-libgsystem.am
+++ /dev/null
@@ -1,31 +0,0 @@
-# Copyright (C) 2012 Colin Walters walt...@verbum.org
-#
-# This library is free software; you can redistribute it and/or
-# modify it under the terms of the GNU Lesser General Public
-# License as published by the Free Software Foundation; either
-# version 2 of the License, or (at your option) any later version.
-#
-# This library is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
-# Lesser General Public License for more details.
-#
-# You should have received a copy of the GNU Lesser General Public
-# License along with this library; if not, write to the
-# Free Software Foundation, Inc., 59 Temple Place - Suite 330,
-# Boston, MA 02111-1307, USA.
-
-noinst_LTLIBRARIES += libgsystem.la
-
-EXTRA_DIST += $(libgsystem_srcpath)/README
-
-libgsystem_la_SOURCES = \
- $(libgsystem_srcpath)/gsystem-local-alloc.h \
- $(libgsystem_srcpath)/gsystem-local-alloc.c \
- $(libgsystem_srcpath)/gsystem-file-utils.h \
- $(libgsystem_srcpath)/gsystem-file-utils.c \
- $(libgsystem_srcpath)/libgsystem.h \
- $(NULL)
-
-libgsystem_la_CFLAGS = $(AM_CFLAGS) $(libgsystem_cflags)
-libgsystem_la_LIBADD = $(libgsystem_libs)
diff --git a/src/libgsystem/README b/src/libgsystem/README
deleted file mode 100644
index 0694940..000
--- a/src/libgsystem/README
+++ /dev/null
@@ -1,7 +0,0 @@
-libgsystem is intended to be used as a git external for components
-that depend on GLib, but accept a hard dependency on things which are
-difficult to do in GLib itself. For
Re: __attribute__ ((cleanup) patch
On Thu, 2012-10-18 at 11:19 -0400, Pavel Simerda wrote: From: Colin Walters walt...@verbum.org Hi, Hi, thanks for a quick reply. https://bugzilla.gnome.org/show_bug.cgi?id=685440 has a patch which just landed, but I wanted to give wider discussion to this, because it's a very important infrastructural change. First, one thing that came up is a concern about a GCC hard dependency. Yes. My understanding is that LLVM implements this too. We have been too conservative to adopt the C99 standard and now we are brave enough to go for non-standard compiler features. We already use some C99 features; basic stuff like __func__ and snprintf(). I don't have a problem with using -std=c99 to build stuff, but it seems like either GCC is already exposing those features with -std=gnu89. The one thing I *don't* like in c99 is the mixed declarations and code; that clutters up the code horribly. For compilers which implement C++, this is easy to support. I don't deny this, but this is not how the question stands, currently. And since NetworkManager is highly tied to Linux NetworkManager is tied to Linux, now, but in my opinion not as highly as it might seem at the first glance. Dan might disagree. It is not a goal of NetworkManager to actively support other platforms. If there was a nice, easy way to support *BSD (and with the platform stuff maybe there would be) then we'd entertain the patch, but we have enough things to work on at the moment than BSD support. it's not like we have to care about MSVC. Nobody's talking about MSVC here. Now, I plan to do a followup patch pretty soon which would let us get rid of a *lot* of g_free() You can get rid of g_free's on small strings for free with a pretty standard way through g_alloca, for example. For that, this is overkill. That doesn't work for allocated values passed back from other functions, which happens quite a bit. But the libgsystem stuff does work for that AFAIK. ie: char *foo; call_some_func (foo); do_something g_free (foo); cannot be made simpler with g_alloca(). /g_object_unref() calls. For this, I would need some more detailed information. This is just not enough for merging someone's experiments that don't even have any documentation. I admit it is tempting to use a real important project for one's experiments but it might not be the right direction for the project. I don't consider this an experiment at this point. We're unlikely to convert to C++ any time soon, and anything that makes C easier to code, reduces programmer errors and memory leaks, and cleans up the existing code is a win, as long as it's not too complicated. Given that this stuff is *less* complicated than the existing code, it's a win. Yes, there is a bit of education required, but perhaps we could also work on making some things compile-time errors instead of runtime gotchas. conservative estimate, around 1500 lines. I've been using libgsystem cleanup extensively in several projects, and it's been a massive win. Then why it wasn't good enough for Glib but is good enough for NetworkManager? Why I don't see a link to elaborate explanation and documentation? Success stories. More information? Other examples of projects using this in C are systemd and upstart. The downside is that this is kind of a one-way door. Once done, it'd be immensely tedious to try to go back and add them again. That's exactly the problem I have with such quick inclusions without a proper discussion. However, the benefit to the code is huge, I'm not yet even convinced about this because of total lack of documentation to be found right away. And I'm not convinced that switching to a non-standard programming language is the price to pay. and even better - trust me, it makes writing C fun again =) You mean writing a language incompatible with the C standards :). Changing the programming language for the whole project in a forward-only compatible manner is a big decision and I'm not really happy with the way it has been done. We use GCC. We don't use obscure features of GCC. We don't adhere blindly to C standards, we deviate where it makes sense. Yes, this issue is up for discussion, but I also haven't heard anything that makes me think a revert is in order. Dan So, I haven't changed my mind. I propose the following path: 1) Revert 2) Document including the reasons why this cannot be added to Glib but should be added to NetworkManager 3) Discuss 4) Decide I don't think this should be a one-week process, so I personally insist on starting with the step (1). I would like to discourage this practice for future decision making regarding such huge one-way changes. Cheers, Pavel ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
On Thu, 2012-10-18 at 11:19 -0400, Pavel Simerda wrote:
I'm not yet even convinced about this because of total lack of documentation
to be
found right away.
What do you think about this patch? Does it help address your concerns?
Is there anything that could be clearer?
From a6f48a9720aa7b01b8abee1fc0eb78230b273485 Mon Sep 17 00:00:00 2001
From: Colin Walters walt...@verbum.org
Date: Thu, 18 Oct 2012 13:11:04 -0400
Subject: [PATCH] Document more
Thanks to Pavel Simerda psime...@redhat.com for bringing this up.
---
gsystem-local-alloc.c | 48
gsystem-local-alloc.h | 43 +++
2 files changed, 91 insertions(+)
diff --git a/gsystem-local-alloc.c b/gsystem-local-alloc.c
index 1bbea90..b19a329 100644
--- a/gsystem-local-alloc.c
+++ b/gsystem-local-alloc.c
@@ -22,6 +22,54 @@
#include gsystem-local-alloc.h
+/**
+ * SECTION:gslocalalloc
+ * @title: GSystem local allocation
+ * @short_description: Release local variables automatically when they go out of scope
+ *
+ * These macros leverage the GCC extension __attribute__ ((cleanup))
+ * to allow calling a cleanup function such as g_free() when a
+ * variable goes out of scope. See ulink
+ * url=http://gcc.gnu.org/onlinedocs/gcc/Variable-Attributes.html;
+ * for more information on the attribute.
+ *
+ * The provided macros make it easy to use the cleanup attribute for types
+ * that come with GLib. The primary two are #gs_lfree and #gs_lobj,
+ * which correspond to g_free() and g_object_unref(), respectively.
+ *
+ * The rationale behind this is that particularly when handling error
+ * paths, it can be very tricky to ensure the right variables are
+ * freed. With this, one simply applies gs_lobj to a
+ * locally-allocated #GFile for example, and it will be automatically
+ * unreferenced when it goes out of scope.
+ *
+ * Note - you should only use these macros for emphasisstack
+ * allocated/emphasis variables. They don't provide garbage
+ * collection or let you avoid freeing things. They're simply a
+ * compiler assisted deterministic mechanism for calling a cleanup
+ * function when a stack frame ends.
+ *
+ * example id=gs-lfreetitleCalling g_free automatically/title
+ * programlisting
+ *
+ * GFile *
+ * create_file (GError **error)
+ * {
+ * gs_lfree char *random_id = NULL;
+ *
+ * if (!prepare_file (error))
+ * return NULL;
+ *
+ * random_id = alloc_random_id ();
+ *
+ * return create_file_real (error);
+ * // Note that random_id is freed here automatically
+ * }
+ * /programlisting
+ * /example
+ *
+ */
+
void
gs_local_free (void *loc)
{
diff --git a/gsystem-local-alloc.h b/gsystem-local-alloc.h
index 24e7ca2..28d857f 100644
--- a/gsystem-local-alloc.h
+++ b/gsystem-local-alloc.h
@@ -25,16 +25,59 @@
G_BEGIN_DECLS
+/* These functions shouldn't be invoked directly;
+ * they are stubs that:
+ * 1) Take a pointer to the location (typically itself a pointer).
+ * 2) Provide %NULL-safety where it doesn't exist already (e.g. g_object_unref)
+ */
void gs_local_free (void *loc);
void gs_local_obj_unref (void *loc);
void gs_local_variant_unref (void *loc);
void gs_local_ptrarray_unref (void *loc);
void gs_local_hashtable_unref (void *loc);
+/**
+ * gs_lfree:
+ *
+ * Call g_free() on a variable location when it goes out of scope.
+ */
#define gs_lfree __attribute__ ((cleanup(gs_local_free)))
+
+/**
+ * gs_lobj:
+ *
+ * Call g_object_unref() on a variable location when it goes out of
+ * scope. Note that unlike g_object_unref(), the variable may be
+ * %NULL.
+ */
#define gs_lobj __attribute__ ((cleanup(gs_local_obj_unref)))
+
+/**
+ * gs_lvariant:
+ *
+ * Call g_variant_unref() on a variable location when it goes out of
+ * scope. Note that unlike g_variant_unref(), the variable may be
+ * %NULL.
+ */
#define gs_lvariant __attribute__ ((cleanup(gs_local_variant_unref)))
+
+/**
+ * gs_lptrarray:
+ *
+ * Call g_ptr_array_unref() on a variable location when it goes out of
+ * scope. Note that unlike g_ptr_array_unref(), the variable may be
+ * %NULL.
+
+ */
#define gs_lptrarray __attribute__ ((cleanup(gs_local_ptrarray_unref)))
+
+/**
+ * gs_lhash:
+ *
+ * Call g_hash_table_unref() on a variable location when it goes out
+ * of scope. Note that unlike g_hash_table_unref(), the variable may
+ * be %NULL.
+ */
#define gs_lhash __attribute__ ((cleanup(gs_local_hashtable_unref)))
G_END_DECLS
--
1.7.11.7
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: __attribute__ ((cleanup) patch
On Thu, 2012-10-18 at 11:28 -0500, Dan Williams wrote: I don't consider this an experiment at this point. Well, I do think Pavel has a point; it's true that NetworkManager is an order of magnitude more code than any other project I've used it in before, and it's larger in terms of people too. Yes, there is a bit of education required, but perhaps we could also work on making some things compile-time errors instead of runtime gotchas. Would require a GCC plugin. But the way I see it is these patches don't *regress* type safety - the compiler isn't going to warn you right now if you call g_free (some_object) either. Anyways, bottom line: given the benefits, my goal here is to figure out where the bar is in the opinion of the NetworkManager developers, and do what I need to reach it. ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
