# [RFC] Cleaning up die() error messages 13 messages from 2005-10-10 to 2005-10-12. Participants: Elfyn McBratney, Junio C Hamano, Daniel Barkalow, Alex Riesen, H. Peter Anvin, Matthias Urlichs. Thread: https://gitlist.dev/t/2056 ## Elfyn McBratney, 2005-10-10 10:50 Subject: [RFC] Cleaning up die() error messages Message-ID: <20051010105008.GB30202@gentoo.org> URL: https://gitlist.dev/e/20051010105008.GB30202%40gentoo.org ``` Hello git list, I've started working on cleaning up the various die() error messages found throughout git, and had a few thoughts along the way. Currently, I've been adding missing program name prefixes, and quoting path names (e.g., "%s" -> "'%s'"), but before I go any further, I'm wondering if this is a) desired, or perhaps b) superfluous? This may be best discussed with along-side the patch - which'll follow shortly. ;) Also, I got to thinking whether it might be an idea to use the following idiom in the code: [shell scripts] prog="`basename $0`" .. foo || die "${prog}: foo failed" [C sources] static char *prog; .. static inline void set_prog_name (char *argv0) { prog = strrchr(argv0, '/'); if (prog) prog++; else prog = argv0; } .. int main (int argc, char **argv) { set_prog_name(argv[0]); .. if (!do_bar()) die("%s: do_bar() failed", prog); .. } The idea behind this being that, if any of the git programs get renamed (again :) there won't be a need for s/git-foo/git-bar/g just to fix-up die() error messages, and it'll also shave a *bit* off of the size of the compiled binaries. ;) (Of course, the C parts (`prog' and `set_prog_name()') would go into a header, and not in every single C source file. ;) So, any thoughts/comments/flames? :) Best, Elfyn -- Elfyn McBratney Gentoo Developer/Perl Team Lead beu/irc.freenode.net http://dev.gentoo.org/~beu/ +------------O.o--------------------- http://dev.gentoo.org/~beu/pubkey.asc PGP Key ID: 0x69DF17AD PGP Key Fingerprint: DBD3 B756 ED58 B1B4 47B9 B3BD 8D41 E597 69DF 17AD ``` ## Junio C Hamano, 2005-10-10 19:04 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: <7vzmph42j2.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vzmph42j2.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20051010105008.GB30202@gentoo.org> ``` Elfyn McBratney writes: > (Of course, the C parts (`prog' and `set_prog_name()') would go into a > header, and not in every single C source file. ;) > > So, any thoughts/comments/flames? :) I do not have objections to either one, except I tend to prefer programs that tells its full path when erroring out, which helps me identify "Oops, my path was screwed up and I am not testing the right one" case. One thing to keep in mind is how badly this C part might interact with the libification effort going on underwater. Since current code Smurf is working on is based on 0.99.6 and many small pieces need to be reviewed anyway, I am not so much worried about forward porting the changes. But some die()s that are in the parts that will be moved to the common library code would also want to use this prog global somehow. But that would not be too much of a problem. Worst case, we force the library clients to do set_prog_name(), or initialize prog to "(unnamed)", or do both. ``` ## Daniel Barkalow, 2005-10-10 20:14 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0510101603520.23242%40iabervon.org In-Reply-To: <20051010105008.GB30202@gentoo.org> ``` On Mon, 10 Oct 2005, Elfyn McBratney wrote: > [C sources] > static char *prog; > .. > static inline void set_prog_name (char *argv0) > { > prog = strrchr(argv0, '/'); > if (prog) > prog++; > else > prog = argv0; > } > .. > int main (int argc, char **argv) > { > set_prog_name(argv[0]); > .. > if (!do_bar()) > die("%s: do_bar() failed", prog); > .. > } Just put set_prog_name() next to die(), and have die() write "prog: " at the beginning if set_prog_name() got called. Assuming it's useful at all to show the name of the program, it's not going to be less useful if the error is actually found in a library function called by the program. -Daniel *This .sig left intentionally blank* ``` ## Alex Riesen, 2005-10-11 15:02 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: <81b0412b0510110802lbcdebe0m17bce7ca81ea76d2@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0510110802lbcdebe0m17bce7ca81ea76d2%40mail.gmail.com In-Reply-To: <20051010105008.GB30202@gentoo.org> ``` On 10/10/05, Elfyn McBratney wrote: > int main (int argc, char **argv) > { > set_prog_name(argv[0]); I'd also use readlink on /proc/self/exe by default (if set_prog_name _not_ called). It simplifies the code at least on linux, and makes possible very slow transition for other platforms. So you don't have to update each and every .c file containing "main[[:space:]]*(" ;) ``` ## H. Peter Anvin, 2005-10-11 16:11 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: <434BE41F.4060909@zytor.com> URL: https://gitlist.dev/e/434BE41F.4060909%40zytor.com In-Reply-To: <81b0412b0510110802lbcdebe0m17bce7ca81ea76d2@mail.gmail.com> ``` Alex Riesen wrote: > On 10/10/05, Elfyn McBratney wrote: > >> int main (int argc, char **argv) >> { >> set_prog_name(argv[0]); > > > I'd also use readlink on /proc/self/exe by default (if set_prog_name > _not_ called). > It simplifies the code at least on linux, and makes possible very slow > transition for other platforms. So you don't have to update each and > every .c file containing "main[[:space:]]*(" ;) It's really better just to put the stuff at the beginning of each main. If that's too annoying, librarize main and rename your mains "git_main". -hpa ``` ## Matthias Urlichs, 2005-10-11 19:48 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: URL: https://gitlist.dev/e/pan.2005.10.11.19.48.04.675482%40smurf.noris.de In-Reply-To: <7vzmph42j2.fsf@assigned-by-dhcp.cox.net> ``` Hi, Junio C Hamano wrote: > One thing to keep in mind is how badly this C part might > interact with the libification effort going on underwater. Not too badly. > Since current code Smurf is working on is based on 0.99.6 and I've merged it up once already; will do that again soon. > many small pieces need to be reviewed anyway, I am not so much > worried about forward porting the changes. I've also mostly succeeded in keeping the individual patches clean so that everything still builds (and verifies), so it might be easiest to just merge with it. ;-) But we'll cross that bridge when we get to it. > But some die()s that > are in the parts that will be moved to the common library code > would also want to use this prog global somehow. IMHO, common library code should not be allowed to die. (Yes, that does imply replacing all the xmalloc() calls.) My library effort has a buffer for the (first) error message. The caller can elect to suppress printing it, so that it can be formatted appropriately. Python, for instance, will wrap errors in an exception. The way I structured it so far, a C program would do git_env = git_env_new(); die_if_null(git_env()); git_env->print_error = 0; [...] git_whatever(git_env, ...); if (git_env->error) { fprintf(stderr, "%s: doing whatever: %s\n", my_program_name, git_env->error); git_env_clear_error(git_env); goto whatever_bad_so_go_clean_up; } -- Matthias Urlichs | {M:U} IT Design @ m-u-it.de | smurf@smurf.noris.de Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de - - A hundred mouths, a hundred tongues, And throats of brass, inspired with iron lungs. -- Virgil ``` ## H. Peter Anvin, 2005-10-11 20:50 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: <434C2590.3040107@zytor.com> URL: https://gitlist.dev/e/434C2590.3040107%40zytor.com In-Reply-To: ``` Matthias Urlichs wrote: > > IMHO, common library code should not be allowed to die. > (Yes, that does imply replacing all the xmalloc() calls.) > The sane way to do this is probably to call an overridable git_die() function, which can be specified by the user to use longjmp(), to use exceptions, or do something else appropriately. However, a much bigger problem is cleanup. -hpa ``` ## Matthias Urlichs, 2005-10-12 01:20 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: URL: https://gitlist.dev/e/pan.2005.10.12.01.20.17.917829%40smurf.noris.de In-Reply-To: <434C2590.3040107@zytor.com> ``` H. Peter Anvin wrote: > The sane way to do this is probably to call an overridable git_die() > function, which can be specified by the user to use longjmp(), to use > exceptions, or do something else appropriately. > I thought about doing something like that, but ... > However, a much bigger problem is cleanup. > ... exactly. -- Matthias Urlichs | {M:U} IT Design @ m-u-it.de | smurf@smurf.noris.de Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de - - BOFH excuse #282: High altitude condensation from U.S.A.F prototype aircraft has contaminated the primary subnet mask. Turn off your computer for 9 days to avoid damaging it. ``` ## H. Peter Anvin, 2005-10-12 03:18 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: <434C8095.4080201@zytor.com> URL: https://gitlist.dev/e/434C8095.4080201%40zytor.com In-Reply-To: ``` Matthias Urlichs wrote: > > I thought about doing something like that, but ... >>However, a much bigger problem is cleanup. > > ... exactly. > I thought about this, and probably the sanest way is to wrap malloc() with something that creates a linked list of allocations. If we abort, we can unwind the linked list and free all allocations. -hpa ``` ## Junio C Hamano, 2005-10-12 04:25 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: <7vd5mbqs2x.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vd5mbqs2x.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <434C8095.4080201@zytor.com> ``` "H. Peter Anvin" writes: > I thought about this, and probably the sanest way is to wrap malloc() > with something that creates a linked list of allocations. If we abort, > we can unwind the linked list and free all allocations. You can free() the allocated memory with something like that, but it is likely that the aborted function would have already created some linked structure out of those memory blocks, and cleaning up that you would need the real exception handling wouldn't you? ``` ## H. Peter Anvin, 2005-10-12 04:40 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: <434C93C5.9040505@zytor.com> URL: https://gitlist.dev/e/434C93C5.9040505%40zytor.com In-Reply-To: <7vd5mbqs2x.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > "H. Peter Anvin" writes: > >>I thought about this, and probably the sanest way is to wrap malloc() >>with something that creates a linked list of allocations. If we abort, >>we can unwind the linked list and free all allocations. > > You can free() the allocated memory with something like that, > but it is likely that the aborted function would have already > created some linked structure out of those memory blocks, and > cleaning up that you would need the real exception handling > wouldn't you? > Not unless any of those linked structures can be reached from outside the git library. -hpa ``` ## Junio C Hamano, 2005-10-12 05:02 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: <7vek6rnx8z.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vek6rnx8z.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <434C93C5.9040505@zytor.com> ``` "H. Peter Anvin" writes: > Not unless any of those linked structures can be reached from outside > the git library. OK. How about things like open file descriptors and mmap'ed regions then? That is: fd = open(); xmalloc(); /* may die now */ close(fd); I think in the long run we would be better off if we properly dealt with the case where (x)malloc returns NULL. Of course, that would make libification a bigger task, so we do not have to do it from day one. ``` ## Matthias Urlichs, 2005-10-12 06:04 Subject: Re: [RFC] Cleaning up die() error messages Message-ID: <20051012060436.GA567@kiste.smurf.noris.de> URL: https://gitlist.dev/e/20051012060436.GA567%40kiste.smurf.noris.de In-Reply-To: <434C8095.4080201@zytor.com> ``` Hi, H. Peter Anvin: > >I thought about doing something like that, but ... > >>However, a much bigger problem is cleanup. > > > >... exactly. > > I thought about this, and probably the sanest way is to wrap malloc() > with something that creates a linked list of allocations. If we abort, > we can unwind the linked list and free all allocations. > There already is a malloc library that does this, plus it can call cleanup for you -- there's more to cleaning up than freeing memory. :-/ Let's see if I can actually find it again. On the other hand, I wonder if the overhead when managing data structures like that really offsets the additional work we'd need to do otherwise, which is simply checking a few more return values. -- Matthias Urlichs | {M:U} IT Design @ m-u-it.de | smurf@smurf.noris.de Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de - - BOFH excuse #381: Robotic tape changer mistook operator's tie for a backup tape. ```