threads / discuss / 11295

config.c fixes

Subject: config.c fixes

## tl;dr

3 messages between Dec 14, 2007 and Dec 14, 2007.

replies: 2people: 2as markdown or json

Kristian Høgsberg· Dec 14, 2007, 19:28 UTC · lore

Hi,

[ This should have gone out as a cover letter for the patch series
  but some how I didn't manage to make that work ]

While strace'ing builtin-clone I saw this horror:

... write(3, "remote", 6) = 6 write(3, " = ", 3) = 3 write(3, "o", 1) = 1 write(3, "r", 1) = 1 write(3, "i", 1) = 1 write(3, "g", 1) = 1 write(3, "i", 1) = 1 write(3, "n", 1) = 1 write(3, "\n", 1) = 1 munmap(0xb7f72000, 102) = 0 close(3) = 0 close(3) = -1 EBADF (Bad file descriptor) ...

That's just terrible. And hey, it turns out the code is terrible too. I think the best solution is to just parse up the entire config file up front and keep it in a data structure, make the changes and then write it all out at the end. That makes it cheap to make a series of changes too:

	config = open_config(file);
	set_key(config, key1, value1);
	set_key(config, key2, value2);
	set_key(config, key3, value3);
	commit_config(config);

Or something. Anyway, for 1.5.4 I wrote the following two patches that fixes the 1-bytes writes and the double close.

 config.c |  113 +++++++++++++++++++++++--------------------------------------
 1 files changed, 43 insertions(+), 70 deletions(-)

cheers, Kristian

Junio C Hamano· Dec 14, 2007, 19:43 UTC · re: Kristian Høgsberg · lore

Re: config.c fixes

Kristian Høgsberg <krh@redhat.com> writes:

> While strace'ing builtin-clone I saw this horror:
> I think the best solution is to just parse up the entire config file
> up front and keep it in a data structure, make the changes and then
> write it all out at the end.

Yeah, that was what I suggested a few times when other people have done config writing side, but without successfully getting past their skulls (it is not Linus's nor my code). It's about time somebody started to clean up that mess.

The timing is a bit unfortunate, though. I would have preferred to have a week or so to cook this in 'next' before merging it part of -rc0.

Kristian Høgsberg· Dec 14, 2007, 20:19 UTC · re: Junio C Hamano · lore

Re: config.c fixes

On Fri, 2007-12-14 at 11:43 -0800, Junio C Hamano wrote:

Show 14 quoted lines

> Kristian Høgsberg <krh@redhat.com> writes:
> 
> > While strace'ing builtin-clone I saw this horror:
> > I think the best solution is to just parse up the entire config file
> > up front and keep it in a data structure, make the changes and then
> > write it all out at the end.
> 
> Yeah, that was what I suggested a few times when other people have done
> config writing side, but without successfully getting past their skulls
> (it is not Linus's nor my code).  It's about time somebody started to
> clean up that mess.
> 
> The timing is a bit unfortunate, though.  I would have preferred to have
> a week or so to cook this in 'next' before merging it part of -rc0.

Right, what I was describing above was more of a long term thing. I sent two patches more suitable for 1.5.4 that fixes the double close and the 1-byte writes in a less intrusive way. Except as the Johanneses point out, I can't use the lock as a local variable, but must allocate so the atexit handler doesn't break. If it has to be allocated, the API should help/enforce that. I've sent out a couple of new patches that fixes this.

Kristian