Re: [PATCH libxkbcommon] keymap: add xkb_keymap_new_from_memory()
I guess I still question why you are not just calling yy_scan_bytes. If you look at the output of flex it contains the source code for yy_scan_bytes, which shows it doing *exactly* the same thing (allocating a buffer 2 bytes larger, copying the memory block, setting the nulls, then calling yy_scan_buffer, and sets things up so resetting the scanner deletes the temporary buffer). It just seems to me that this code serves no purpose, and that if flex is ever "fixed" to not require the bytes (most likely by making it reuse the code to read blocks from files to read blocks from the source buffer), that this patch would then not use this fix and have to be removed anyway... ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libxkbcommon] keymap: add xkb_keymap_new_from_memory()
Hi Ran
On Thu, Mar 14, 2013 at 2:14 PM, Ran Benita wrote:
> On Wed, Mar 13, 2013 at 07:28:17PM +0200, Ran Benita wrote:
>> On Mon, Mar 11, 2013 at 12:53:39PM +0100, David Herrmann wrote:
>> > The current API doesn't allow the caller to create keymaps from mmap()'ed
>> > files. The problem is, xkb_keymap_new_from_string() requires a terminating
>> > 0 byte. However, there is no way to guarantee that when using mmap() so a
>> > user currently has to copy the whole file just to get the terminating zero
>> > byte (assuming they cannot use xkb_keymap_new_from_file()).
>> >
>> > This adds a new entry xkb_keymap_new_from_memory() which takes a memory
>> > location and the size in bytes.
>> >
>> > Internally, we depend on yy_scan_{string,byte}() helpers. According to
>> > flex documentation these already copy the input string because they are
>> > wrappers around yy_scan_buffer().
>> > yy_scan_buffer() on the other hand has some insane requirements. The
>> > buffer must be writeable and the last two bytes must be ASCII-NUL. But the
>> > buffer may contain other 0 bytes just fine.
>> >
>> > Because we don't want these constraints in our public API,
>> > xkb_keymap_new_from_memory() needs to create a copy of the input memory.
>> > But it then calls yy_scan_buffer() directly. Hence, we have the same
>> > number of buffer-copies as with *_from_string() but without the
>> > terminating 0 requirement.
>> >
>> > Maybe some day we no longer depend on flex and can have a zero-copy API. A
>> > user could mmap() a file and it would get parsed right from this buffer.
>> > But until then, we shouldn't expose this limitation in the API but instead
>> > provide an API that some day can work with zero-copy.
>> >
>> > Signed-off-by: David Herrmann
>>
>> Patch looks good to me. Too bad we need the extra entry point.
>>
>> As a side note to your last point: replacing flex is pretty easy, in
>> fact I've done it some months ago. I decided against it though because
>> flex files are nice and declarative and there wasn't a good reason to
>> replace it. But I've done a quick rebase now on top of my working branch
>> and your patch:
>> https://github.com/bluetech/libxkbcommon/commits/scanner-wip
>> Maybe it's worth it now.
>
> I've done a V2 and now I'm happy with it, so no more "wip":
> https://github.com/bluetech/libxkbcommon/commits/scanner
The patch looks good. I don't have time to review it properly, sorry.
But at least you can change '\"' to '"'. No backslash needed ;)
Regarding _from_memory(), Daniel said he is fine with it but wants to
rethink the name.
Thanks
David
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libxkbcommon] keymap: add xkb_keymap_new_from_memory()
On Wed, Mar 13, 2013 at 07:28:17PM +0200, Ran Benita wrote:
> On Mon, Mar 11, 2013 at 12:53:39PM +0100, David Herrmann wrote:
> > The current API doesn't allow the caller to create keymaps from mmap()'ed
> > files. The problem is, xkb_keymap_new_from_string() requires a terminating
> > 0 byte. However, there is no way to guarantee that when using mmap() so a
> > user currently has to copy the whole file just to get the terminating zero
> > byte (assuming they cannot use xkb_keymap_new_from_file()).
> >
> > This adds a new entry xkb_keymap_new_from_memory() which takes a memory
> > location and the size in bytes.
> >
> > Internally, we depend on yy_scan_{string,byte}() helpers. According to
> > flex documentation these already copy the input string because they are
> > wrappers around yy_scan_buffer().
> > yy_scan_buffer() on the other hand has some insane requirements. The
> > buffer must be writeable and the last two bytes must be ASCII-NUL. But the
> > buffer may contain other 0 bytes just fine.
> >
> > Because we don't want these constraints in our public API,
> > xkb_keymap_new_from_memory() needs to create a copy of the input memory.
> > But it then calls yy_scan_buffer() directly. Hence, we have the same
> > number of buffer-copies as with *_from_string() but without the
> > terminating 0 requirement.
> >
> > Maybe some day we no longer depend on flex and can have a zero-copy API. A
> > user could mmap() a file and it would get parsed right from this buffer.
> > But until then, we shouldn't expose this limitation in the API but instead
> > provide an API that some day can work with zero-copy.
> >
> > Signed-off-by: David Herrmann
>
> Patch looks good to me. Too bad we need the extra entry point.
>
> As a side note to your last point: replacing flex is pretty easy, in
> fact I've done it some months ago. I decided against it though because
> flex files are nice and declarative and there wasn't a good reason to
> replace it. But I've done a quick rebase now on top of my working branch
> and your patch:
> https://github.com/bluetech/libxkbcommon/commits/scanner-wip
> Maybe it's worth it now.
I've done a V2 and now I'm happy with it, so no more "wip":
https://github.com/bluetech/libxkbcommon/commits/scanner
Ran
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libxkbcommon] keymap: add xkb_keymap_new_from_memory()
On Mon, Mar 11, 2013 at 12:53:39PM +0100, David Herrmann wrote:
> The current API doesn't allow the caller to create keymaps from mmap()'ed
> files. The problem is, xkb_keymap_new_from_string() requires a terminating
> 0 byte. However, there is no way to guarantee that when using mmap() so a
> user currently has to copy the whole file just to get the terminating zero
> byte (assuming they cannot use xkb_keymap_new_from_file()).
>
> This adds a new entry xkb_keymap_new_from_memory() which takes a memory
> location and the size in bytes.
>
> Internally, we depend on yy_scan_{string,byte}() helpers. According to
> flex documentation these already copy the input string because they are
> wrappers around yy_scan_buffer().
> yy_scan_buffer() on the other hand has some insane requirements. The
> buffer must be writeable and the last two bytes must be ASCII-NUL. But the
> buffer may contain other 0 bytes just fine.
>
> Because we don't want these constraints in our public API,
> xkb_keymap_new_from_memory() needs to create a copy of the input memory.
> But it then calls yy_scan_buffer() directly. Hence, we have the same
> number of buffer-copies as with *_from_string() but without the
> terminating 0 requirement.
>
> Maybe some day we no longer depend on flex and can have a zero-copy API. A
> user could mmap() a file and it would get parsed right from this buffer.
> But until then, we shouldn't expose this limitation in the API but instead
> provide an API that some day can work with zero-copy.
>
> Signed-off-by: David Herrmann
Patch looks good to me. Too bad we need the extra entry point.
As a side note to your last point: replacing flex is pretty easy, in
fact I've done it some months ago. I decided against it though because
flex files are nice and declarative and there wasn't a good reason to
replace it. But I've done a quick rebase now on top of my working branch
and your patch:
https://github.com/bluetech/libxkbcommon/commits/scanner-wip
Maybe it's worth it now.
Thanks,
Ran
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libxkbcommon] keymap: add xkb_keymap_new_from_memory()
Hi Bill On Mon, Mar 11, 2013 at 7:06 PM, Bill Spitzak wrote: > Okay looking more at your patch and the output of flex, I think you can just > call yy_scan_bytes. It does a copy, true, but that is exactly what your code > is doing. And it does it by reusing code that is already in the flex output. yy_scan_bytes saves us a direct call to malloc(), nothing more. I thought it's better to do that ourselves so we actually see that there is a copy involved and not hide it in flex. For instance if we ever add an mmap() interface or if we need it for something else internally, we can reuse XkbParseBuffer() and don't need another of these helpers. We could even remove XkbParseString() and use XkbParseBuffer() instead. Anyway, I'm open to change this to use yy_scan_bytes() instead if that's preferred. Regards David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libxkbcommon] keymap: add xkb_keymap_new_from_memory()
Okay looking more at your patch and the output of flex, I think you can
just call yy_scan_bytes. It does a copy, true, but that is exactly what
your code is doing. And it does it by reusing code that is already in
the flex output.
And if flex is ever fixed so the two nulls are not needed, it seems
likely yy_scan_bytes will be fixed to not do the copy (a fix might be to
copy "blocks" into the parsing buffer, which is what it is doing for files).
I did fix our code which was adding +1 to strlen when calling
yy_scan_bytes, due to a misunderstanding of the flex documentation. It
obviously works without this.
Bill Spitzak wrote:
Oops, never mind, it looks like you have already analyzed yy_scan_byte
and found it calls yy_scan_buffer internally (!!!). Oh well.
David Herrmann wrote:
Internally, we depend on yy_scan_{string,byte}() helpers. According to
flex documentation these already copy the input string because they are
wrappers around yy_scan_buffer().
yy_scan_buffer() on the other hand has some insane requirements. The
buffer must be writeable and the last two bytes must be ASCII-NUL. But
the
buffer may contain other 0 bytes just fine.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libxkbcommon] keymap: add xkb_keymap_new_from_memory()
Oops, never mind, it looks like you have already analyzed yy_scan_byte
and found it calls yy_scan_buffer internally (!!!). Oh well.
David Herrmann wrote:
Internally, we depend on yy_scan_{string,byte}() helpers. According to
flex documentation these already copy the input string because they are
wrappers around yy_scan_buffer().
yy_scan_buffer() on the other hand has some insane requirements. The
buffer must be writeable and the last two bytes must be ASCII-NUL. But the
buffer may contain other 0 bytes just fine.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libxkbcommon] keymap: add xkb_keymap_new_from_memory()
I know we got flex to work without a terminating null. A quick look at the source code reveals that we are calling this: yy_scan_bytes(buffer, length, scanner); Our code also does this: #define YY_INPUT(a,b,c) // disable read-next-buffer: unused and makes a warning I'm not sure if that is important. David Herrmann wrote: The current API doesn't allow the caller to create keymaps from mmap()'ed files. The problem is, xkb_keymap_new_from_string() requires a terminating 0 byte. However, there is no way to guarantee that when using mmap() so a user currently has to copy the whole file just to get the terminating zero byte (assuming they cannot use xkb_keymap_new_from_file()). ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libxkbcommon] keymap: add xkb_keymap_new_from_memory()
The current API doesn't allow the caller to create keymaps from mmap()'ed
files. The problem is, xkb_keymap_new_from_string() requires a terminating
0 byte. However, there is no way to guarantee that when using mmap() so a
user currently has to copy the whole file just to get the terminating zero
byte (assuming they cannot use xkb_keymap_new_from_file()).
This adds a new entry xkb_keymap_new_from_memory() which takes a memory
location and the size in bytes.
Internally, we depend on yy_scan_{string,byte}() helpers. According to
flex documentation these already copy the input string because they are
wrappers around yy_scan_buffer().
yy_scan_buffer() on the other hand has some insane requirements. The
buffer must be writeable and the last two bytes must be ASCII-NUL. But the
buffer may contain other 0 bytes just fine.
Because we don't want these constraints in our public API,
xkb_keymap_new_from_memory() needs to create a copy of the input memory.
But it then calls yy_scan_buffer() directly. Hence, we have the same
number of buffer-copies as with *_from_string() but without the
terminating 0 requirement.
Maybe some day we no longer depend on flex and can have a zero-copy API. A
user could mmap() a file and it would get parsed right from this buffer.
But until then, we shouldn't expose this limitation in the API but instead
provide an API that some day can work with zero-copy.
Signed-off-by: David Herrmann
---
Makefile.am| 2 ++
src/xkbcomp/scanner.l | 32 +
src/xkbcomp/xkbcomp-priv.h | 4 +++
src/xkbcomp/xkbcomp.c | 45 +++
test/.gitignore| 1 +
test/common.c | 15
test/memorycomp.c | 90 ++
test/test.h| 3 ++
xkbcommon/xkbcommon.h | 15
9 files changed, 207 insertions(+)
create mode 100644 test/memorycomp.c
diff --git a/Makefile.am b/Makefile.am
index 6ecb8f5..7088d67 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -140,6 +140,7 @@ TESTS = \
test/context \
test/rules-file \
test/stringcomp \
+ test/memorycomp \
test/keyseq \
test/log
TESTS_LDADD = libtest.la
@@ -152,6 +153,7 @@ test_context_LDADD = $(TESTS_LDADD)
test_rules_file_CFLAGS = $(AM_CFLAGS) -Wno-declaration-after-statement
test_rules_file_LDADD = $(TESTS_LDADD) -lrt
test_stringcomp_LDADD = $(TESTS_LDADD)
+test_memorycomp_LDADD = $(TESTS_LDADD)
test_keyseq_LDADD = $(TESTS_LDADD)
test_log_LDADD = $(TESTS_LDADD)
test_interactive_LDADD = $(TESTS_LDADD)
diff --git a/src/xkbcomp/scanner.l b/src/xkbcomp/scanner.l
index 5ccf3e9..f30462d 100644
--- a/src/xkbcomp/scanner.l
+++ b/src/xkbcomp/scanner.l
@@ -267,6 +267,38 @@ XkbParseString(struct xkb_context *ctx, const char *string,
return xkb_file;
}
+/*
+ * yy_scan_buffer() requires the last two bytes of \buf to be 0. These two
bytes
+ * are not scanned. Other zero bytes in the buffer are scanned normally,
though.
+ * Due to these terminating zeroes, \length must be greater than 2.
+ * Furthermore, the buffer must be writable and you cannot make any assumptions
+ * about it after the scanner finished.
+ * All this must be guaranteed by the caller of this function!
+ */
+XkbFile *
+XkbParseBuffer(struct xkb_context *ctx, char *buf, size_t length,
+ const char *file_name)
+{
+yyscan_t scanner;
+struct scanner_extra extra;
+YY_BUFFER_STATE state;
+XkbFile *xkb_file;
+
+if (!init_scanner(&scanner, &extra, ctx, file_name))
+return NULL;
+
+xkb_file = NULL;
+state = yy_scan_buffer(buf, length, scanner);
+if (state) {
+xkb_file = parse(ctx, scanner, NULL);
+yy_delete_buffer(state, scanner);
+}
+
+clear_scanner(scanner);
+
+return xkb_file;
+}
+
XkbFile *
XkbParseFile(struct xkb_context *ctx, FILE *file,
const char *file_name, const char *map)
diff --git a/src/xkbcomp/xkbcomp-priv.h b/src/xkbcomp/xkbcomp-priv.h
index b2b40fd..0d35255 100644
--- a/src/xkbcomp/xkbcomp-priv.h
+++ b/src/xkbcomp/xkbcomp-priv.h
@@ -45,6 +45,10 @@ XkbFile *
XkbParseString(struct xkb_context *ctx, const char *string,
const char *file_name);
+XkbFile *
+XkbParseBuffer(struct xkb_context *ctx, char *buf, size_t length,
+ const char *file_name);
+
void
FreeXkbFile(XkbFile *file);
diff --git a/src/xkbcomp/xkbcomp.c b/src/xkbcomp/xkbcomp.c
index 5025dc6..d1a7ffb 100644
--- a/src/xkbcomp/xkbcomp.c
+++ b/src/xkbcomp/xkbcomp.c
@@ -149,6 +149,51 @@ xkb_keymap_new_from_string(struct xkb_context *ctx,
}
XKB_EXPORT struct xkb_keymap *
+xkb_keymap_new_from_memory(struct xkb_context *ctx,
+ const char *memory,
+ size_t length,
+ enum xkb_keymap_format format,
+ enum xkb_keymap_compile_flags flags)
+{
+XkbFile *file;
+struct xkb_keymap *keymap;
+char *buf;
+
