From: Thomas L. <ta...@gm...> - 2008-06-29 19:34:49
|
Looking at commit 11ce57102f89a7dad707d1a2f050749c4507414f (kerofin): +/* Search for a application on $APPDIRPATH + * (~/Apps:/usr/local/apps:/usr/apps) and return a copy of the path found. + * Returns NULL if not found + */ +gchar *find_app(const char *appname) +{ + const gchar *path=g_getenv(appname); + gchar **search; Why does it call g_getenv on appname? Also: + for(i=0; search[i]; i++) + { + app=g_strconcat(search[i], "/", appname, NULL); + if(access(app, X_OK)==0) + goto out; + } Missing a g_free? -- Dr Thomas Leonard ROX desktop / Zero Install GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 |
From: Stephen W. <st...@ke...> - 2008-06-30 06:07:41
|
"Thomas Leonard" <ta...@gm...> wrote: > Looking at commit 11ce57102f89a7dad707d1a2f050749c4507414f (kerofin): > > +/* Search for a application on $APPDIRPATH > + * (~/Apps:/usr/local/apps:/usr/apps) and return a copy of the path found. > + * Returns NULL if not found > + */ > +gchar *find_app(const char *appname) > +{ > + const gchar *path=g_getenv(appname); > + gchar **search; > > Why does it call g_getenv on appname? Should have been g_getenv("APPDIRPATH") > Also: > > + for(i=0; search[i]; i++) > + { > + app=g_strconcat(search[i], "/", appname, NULL); > + if(access(app, X_OK)==0) > + goto out; > + } > > Missing a g_free? I don't think so. app is returned and search is freed before exit. -- Stephen Watson http://www.kerofin.demon.co.uk/ If you read this on a mailing list, send any reply back to the list and not to me. Not even CC. "Bad dog!" "Affirmative!" |
From: Tony H. <h...@re...> - 2008-06-30 12:10:56
|
In <gem...@ke...> Stephen Watson <st...@ke...> wrote: > "Thomas Leonard" <ta...@gm...> wrote: > > > Looking at commit 11ce57102f89a7dad707d1a2f050749c4507414f > > (kerofin): > > > > + for(i=0; search[i]; i++) > > + { > > + app=g_strconcat(search[i], "/", appname, NULL); > > + if(access(app, X_OK)==0) > > + goto out; g_free(app); app = NULL; > > + } > > > > Missing a g_free? > > I don't think so. app is returned and search is freed before exit. It needs to be freed each time the loop fails to find a match though, as I've inserted above. I'm not sure whether setting it to NULL is also necessary without seeing the rest of the code, but this is the safe option. -- TH * http://www.realh.co.uk |
From: Stephen W. <st...@ke...> - 2008-06-30 17:44:55
|
Tony Houghton <h...@re...> wrote: > In <gem...@ke...> > Stephen Watson <st...@ke...> wrote: > > > "Thomas Leonard" <ta...@gm...> wrote: > > > > > Looking at commit 11ce57102f89a7dad707d1a2f050749c4507414f > > > (kerofin): > > > > > > + for(i=0; search[i]; i++) > > > + { > > > + app=g_strconcat(search[i], "/", appname, NULL); > > > + if(access(app, X_OK)==0) > > > + goto out; > g_free(app); > app = NULL; > > > + } > > > > > > Missing a g_free? > > > > I don't think so. app is returned and search is freed before exit. > > It needs to be freed each time the loop fails to find a match though, as > I've inserted above. Oh yes. > I'm not sure whether setting it to NULL is also > necessary It's set to NULL when the loop exits through expiry. -- Stephen Watson http://www.kerofin.demon.co.uk/ If you read this on a mailing list, send any reply back to the list and not to me. Not even CC. Do you mind not farting while I'm saving the world? |
From: Tony H. <h...@re...> - 2008-06-30 13:52:15
|
In <200...@ti...n> Tony Houghton <h...@re...> wrote: > In <gem...@ke...> > Stephen Watson <st...@ke...> wrote: > > > "Thomas Leonard" <ta...@gm...> wrote: > > > > > Looking at commit 11ce57102f89a7dad707d1a2f050749c4507414f > > > (kerofin): > > > > > > + for(i=0; search[i]; i++) > > > + { > > > + app=g_strconcat(search[i], "/", appname, NULL); > > > + if(access(app, X_OK)==0) > > > + goto out; > g_free(app); > app = NULL; > > > + } > > > > > > Missing a g_free? > > > > I don't think so. app is returned and search is freed before exit. > > It needs to be freed each time the loop fails to find a match though, > as I've inserted above. I'm not sure whether setting it to NULL is > also necessary without seeing the rest of the code, but this is the > safe option. Actually where did the code come from? I can't find that commit in the latest pull. I'd like to have a look at it and see if that goto is really appropriate. goto does have its uses but they should be fairly exceptional. It might be better for the code after the loop to be based on: if (app) { ... } else { ... } -- TH * http://www.realh.co.uk |
From: Thomas L. <ta...@gm...> - 2008-06-30 17:02:00
|
2008/6/30 Tony Houghton <h...@re...>: > In <200...@ti...n> > Tony Houghton <h...@re...> wrote: > >> In <gem...@ke...> >> Stephen Watson <st...@ke...> wrote: >> >> > "Thomas Leonard" <ta...@gm...> wrote: >> > >> > > Looking at commit 11ce57102f89a7dad707d1a2f050749c4507414f >> > > (kerofin): >> > > >> > > + for(i=0; search[i]; i++) >> > > + { >> > > + app=g_strconcat(search[i], "/", appname, NULL); >> > > + if(access(app, X_OK)==0) >> > > + goto out; >> g_free(app); >> app = NULL; >> > > + } >> > > >> > > Missing a g_free? >> > >> > I don't think so. app is returned and search is freed before exit. >> >> It needs to be freed each time the loop fails to find a match though, >> as I've inserted above. I'm not sure whether setting it to NULL is >> also necessary without seeing the rest of the code, but this is the >> safe option. > > Actually where did the code come from? I can't find that commit in the > latest pull. I'd like to have a look at it and see if that goto is > really appropriate. goto does have its uses but they should be fairly > exceptional. It might be better for the code after the loop to be based > on: It's from the master branch here: http://kerofin.demon.co.uk/~stephen/git/rox-filer/ -- Dr Thomas Leonard ROX desktop / Zero Install GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 |