Re: Bug: .gitconfig folder
Jeff King p...@peff.net writes: On Wed, May 27, 2015 at 03:38:12PM -0700, Junio C Hamano wrote: The patch was meant to be a tongue-in-cheek tangent that is a vast improvement for cases where we absolutely need to use mmap but does not help the OP at all ;-) I do not think there is any need for the config reader to read the existing file via mmap interface; just open it, strbuf_read() the whole thing (and complain when it cannot) and we should be ok. Or do we write back through the mmaped region or something? No, I think we must never do that in our code because our compat mmap implementation uses pread(). So all maps must be MAP_PRIVATE (and our compat mmap barfs if it is not). I started to go the strbuf_read() route, but it just felt so dirty to change the way the code works only to try to get a better error message. Hmm. I actually thought that we long time ago updated the system to read small loose object files via read(2) instead of mmap(2) purely as an optimization, as mmap(2) is a bad match if you are going to read the whole thing from the beginning to the end anyway, and the why not strbuf_read() the whole configuration file was a suggestion along that line. But apparently we do not have such an optimization in read_object() codepath, perhaps I was hallucinating X-. ... but the config-writing code is such a tangled mess that I don't want to spend the time or risk the regressions. That part I agree with. I was kinda hoping that the previous GSoC would clean it up, but that did not happen. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug: .gitconfig folder
On Wed, May 27, 2015 at 03:24:47PM -0700, Stefan Beller wrote: What is our thinking for after-fact recovery attempts? Like we try the mmap first, if that fails we just use open to get the contents of the file. And when open fails, we can still print a nice error message? For config, I think we could just open and read the file in the first place. The data is not typically very big (and if you have a 3G config file and git barfs with out of memory, I can live with that). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug: .gitconfig folder
On Wed, May 27, 2015 at 03:38:12PM -0700, Junio C Hamano wrote: The patch was meant to be a tongue-in-cheek tangent that is a vast improvement for cases where we absolutely need to use mmap but does not help the OP at all ;-) I do not think there is any need for the config reader to read the existing file via mmap interface; just open it, strbuf_read() the whole thing (and complain when it cannot) and we should be ok. Or do we write back through the mmaped region or something? No, I think we must never do that in our code because our compat mmap implementation uses pread(). So all maps must be MAP_PRIVATE (and our compat mmap barfs if it is not). I started to go the strbuf_read() route, but it just felt so dirty to change the way the code works only to try to get a better error message. So here's my attempt at making it better while still using mmap. The end result is: $ mkdir foo $ git config --file=foo some.key value error: unable to mmap 'foo': Is a directory Having looked through the code, I think the _ideal_ way to implement it would actually be with read() and seek(). We read through the config once (with the normal parser, which wraps stdio) and mark the offsets of chunks we want to copy to the output. Then we mmap the original (under lock, at least, so it shouldn't be racy) and output the existing chunks and any new content in the appropriate order. So ideally writing each chunk would just be seek() and copy_fd(). But our offsets aren't quite perfect. In some cases we read backwards in our mmap to find the right cutoff point. I'm sure this is fixable given sufficient refactoring, but the config-writing code is such a tangled mess that I don't want to spend the time or risk the regressions. [1/4]: read-cache.c: drop PROT_WRITE from mmap of index [2/4]: config.c: fix mmap leak when writing config [3/4]: config.c: avoid xmmap error messages [4/4]: config.c: rewrite ENODEV into EISDIR when mmap fails -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug: .gitconfig folder
Jorge grif...@gmx.es writes: If you have a folder named ~/.gitconfig instead of a file with that name, when you try to run some global config editing command it will fail with a wrong error message: fatal: Out of memory? mmap failed: No such device That indeed is a funny error message. How about this patch? -- 8 -- We show that message with die_errno(), but the OS is ought to know why mmap(2) failed much better than we do. There is no reason for us to say Out of memory? here. Note that mmap(2) fails with ENODEV when the file you specify is not something that can be mmap'ed, so you still need to know that No such device can include cases like having a directory when a regular file is expected, but we can expect that a user who creates a directory to a location where a regular file is expected to be would know what s/he is doing, hopefully ;-) sha1_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sha1_file.c b/sha1_file.c index ccc6dac..551a9e9 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -720,7 +720,7 @@ void *xmmap(void *start, size_t length, release_pack_memory(length); ret = mmap(start, length, prot, flags, fd, offset); if (ret == MAP_FAILED) - die_errno(Out of memory? mmap failed); + die_errno(mmap failed); } return ret; } -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug: .gitconfig folder
On Wed, May 27, 2015 at 01:30:29PM -0700, Junio C Hamano wrote: Jorge grif...@gmx.es writes: If you have a folder named ~/.gitconfig instead of a file with that name, when you try to run some global config editing command it will fail with a wrong error message: fatal: Out of memory? mmap failed: No such device That indeed is a funny error message. How about this patch? -- 8 -- We show that message with die_errno(), but the OS is ought to know why mmap(2) failed much better than we do. There is no reason for us to say Out of memory? here. Note that mmap(2) fails with ENODEV when the file you specify is not something that can be mmap'ed, so you still need to know that No such device can include cases like having a directory when a regular file is expected, but we can expect that a user who creates a directory to a location where a regular file is expected to be would know what s/he is doing, hopefully ;-) sha1_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sha1_file.c b/sha1_file.c index ccc6dac..551a9e9 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -720,7 +720,7 @@ void *xmmap(void *start, size_t length, release_pack_memory(length); ret = mmap(start, length, prot, flags, fd, offset); if (ret == MAP_FAILED) - die_errno(Out of memory? mmap failed); + die_errno(mmap failed); } This is definitely an improvement, but the real failing of that error message is that it does not tell us that ~/.gitconfig is the culprit. I don't think we can do much from xmmap, though; it does not have the filename. It would be nice if we got EISDIR from open() in the first place, but I don't think we can implement that efficiently (if we added an xopen that checked that, it would have to stat() every file we opened). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug: .gitconfig folder
Jeff King p...@peff.net writes: -die_errno(Out of memory? mmap failed); +die_errno(mmap failed); This is definitely an improvement, but the real failing of that error message is that it does not tell us that ~/.gitconfig is the culprit. I don't think we can do much from xmmap, though; it does not have the filename. It would be nice if we got EISDIR from open() in the first place, but I don't think we can implement that efficiently (if we added an xopen that checked that, it would have to stat() every file we opened). The patch was meant to be a tongue-in-cheek tangent that is a vast improvement for cases where we absolutely need to use mmap but does not help the OP at all ;-) I do not think there is any need for the config reader to read the existing file via mmap interface; just open it, strbuf_read() the whole thing (and complain when it cannot) and we should be ok. Or do we write back through the mmaped region or something? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug: .gitconfig folder
On Wed, May 27, 2015 at 3:18 PM, Jeff King p...@peff.net wrote: On Wed, May 27, 2015 at 01:30:29PM -0700, Junio C Hamano wrote: Jorge grif...@gmx.es writes: If you have a folder named ~/.gitconfig instead of a file with that name, when you try to run some global config editing command it will fail with a wrong error message: fatal: Out of memory? mmap failed: No such device That indeed is a funny error message. How about this patch? -- 8 -- We show that message with die_errno(), but the OS is ought to know why mmap(2) failed much better than we do. There is no reason for us to say Out of memory? here. Note that mmap(2) fails with ENODEV when the file you specify is not something that can be mmap'ed, so you still need to know that No such device can include cases like having a directory when a regular file is expected, but we can expect that a user who creates a directory to a location where a regular file is expected to be would know what s/he is doing, hopefully ;-) sha1_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sha1_file.c b/sha1_file.c index ccc6dac..551a9e9 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -720,7 +720,7 @@ void *xmmap(void *start, size_t length, release_pack_memory(length); ret = mmap(start, length, prot, flags, fd, offset); if (ret == MAP_FAILED) - die_errno(Out of memory? mmap failed); + die_errno(mmap failed); } This is definitely an improvement, but the real failing of that error message is that it does not tell us that ~/.gitconfig is the culprit. I don't think we can do much from xmmap, though; it does not have the filename. It would be nice if we got EISDIR from open() in the first place, but I don't think we can implement that efficiently (if we added an xopen that checked that, it would have to stat() every file we opened). What is our thinking for after-fact recovery attempts? Like we try the mmap first, if that fails we just use open to get the contents of the file. And when open fails, we can still print a nice error message? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
