Hi Bruno, Sorry for the delay, I was flat out with CeBIT Australia.
The attached patch is what is in mondo-2.07-2 for Debian. It does not include your suggestions below. I decided to release something (for Debian) to keep some momentum and because I am not sure how much time I'll have in the near future. I hope that is ok. Just some comments on the attached patch: - calling wait_until_software_raids_are_prepped() with a value of 10 rather than 100 gives segfaults for more than one RAID array - wait_until_software_raids_are_prepped() messaes up the screen somewhat if more than one RAID array is present - I've put the sync and sleep back in after the RAID arrays are created regardless how this was done Further comments inline. On Mon, 2006-05-08 at 15:21 +0200, Bruno Cornec wrote: > On Sun, May 07, 2006 at 06:18:27PM +1000, Andree Leidenfrost wrote: > > - replaced fgets with getline() to avoid static strings and thus > > overflows > > Good point ! Cool, so I got that right. ;-) > > I've done some more testing, valgrind is still happy, so I think this is > > as good as it gets at the moment. If there is anything that I've > > overloooked, please let me know. > > Well, in order just to be picky, I would declare each variable on a > single line (personal preference): > > char *token, *string = NULL, *pos, type; > => > char *token; > char *string = NULL; > char *pos; > char *type; > > I find it more readable IMHO. No problem, I'm happy to change this. > > Other point. I'm not sure of the usage of memove and the memory > management it implies. > > I would rather do: > > asprintf(&str2,pos); rather than memmove(string, pos, strlen(string)); > potentially followed by paranoid_free(string); > string = str2; > > if you find it more readable. > > because I'm not not sure what will happen of the first part of the > string, when you will free string later on. I fear a memory leak again. > > I'd prefer to create more tmp variables, it's more work, but it's more > convenient, and thus we know what we allocated, and what to free. > So in clear we should be able to deal nearly only with asprintf and > getline, as memory string functions. > memcpy could still be used for structure copy, but that's another story. Hm, I think the memmove command is watertight. But if you want to standardise on using asprintf pretty much exclusively that's ok with me, too. It's just a bit clumsier (but probably less error-prone, so fair enough). > I wonder if I'll not commit a first set of asprintf changes from trunk > to stable, now that we go to 2.2.0. WDYT ? I think this would be really, really. I am all for 'release early, release often' which in this case would mean to phase the asprintf things in - the code will become safer with every asprintf that we put in. So, go for it! > I can't merge the latest modifications as they do not work, but most of > first where working, and it could be time to migrate them from trunk to > stable. It wil also ease the use of trunk for dealing with the rest of > memory management, minimizing the time to merge the patches from stable > to trunk. > > Also wouldn't getdelim be useful for what you try to do ? Hm, as things are at the moment not really I don't think because we are reading in a whole line and then dissect it. getdelim() basically would allow us to use a different end of line character other than newline, so the surrounding code would have to be changed quite a bit. > Also in the man page of strtok: > > BUGS > Never use these functions. > > WDYT ? ;-) (Again I fear the following free, will it free the whole > memory ? We need to pass to free the pointer to the begining of the > thestring allocated by asprintf or getline) Oops, didn't see that in the manpage. Certainly, the docu here: http://www.gnu.org/software/libc/manual/html_node/Finding-Tokens-in-a-String.html#Finding-Tokens-in-a-String doesn't have this severe warning. I certainly see your point, though. Maybe storing the postion of string in the beginning and freeing that would work as a simple stop gap? (Funny that valgrind doesn't complain about this BTW. <scratch my head>) > Maybe we should write our own mr_strtok function which will do it > correctly ? That may be the best thing to do. > That's where perl is so efficient compared to C :-( Well, yes, C is not exactly designed for string processing whereas Perl (amongst other things) certainly is. > I know I'm surely too picky (as usual) but memory management has been so > strangely handled in mondo, that I would like to change that. That's perfectly cool. You are surely raising valid points and I can learn... > OTOH, I still wonder whether we should write everything in C (when it's > mainly string management, perl is really much better) > > Not to refrain you, but I think it's the right moment to consider that > quite important point, and agree on it before going further. > > Waiting your opinion on this, Fair points you raise indeed. The thing is that when one takes a closer look at mondoarchive and mondorescue they are to a large extend like scripts that call all sorts of other tools all the time, only that the 'scripting language' is actually C. However, this in itself does not have to be a bad thing. In regards to strings processing, Perl would certainly be better suited. But then again, is it really that much string parsing that we do? And would it thus really be worth it? To tell the truth, I'd rather get along without relying on Perl for the rescue media. Maybe good old awk could be used a bit more instead? (And mr_strtok() of course. ;-) ) WDYT? > Greetings > Bruno. I'll see whether I can work on an updated patch incorporating your latest suggestions, but thought I send this through first (including the current patch). Cheers, Andree -- Andree Leidenfrost Sydney - Australia
mondo_2.07-1raid_v2.diff.gz
Description: GNU Zip compressed data
signature.asc
Description: This is a digitally signed message part
