config.c fixes

config.c fixes

[Kristian Høgsberg]

Hi,

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

config.c fixes

[Kristian Høgsberg]

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

Re: config.c fixes

[Junio C Hamano]

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.

Re: config.c fixes

[Kristian Høgsberg]

On Fri, 2007-12-14 at 11:43 -0800, Junio C Hamano wrote:
> 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