Re: [systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
On Thu, 28.11.13 09:32, David Herrmann (dh.herrm...@gmail.com) wrote: +sd_gfx_font *font; +int r; + +ppi = CLAMP(ppi, 10U, 1000U); + +font = calloc(1, sizeof(*font)); font = new0(sd_gfx_font, 1); I actually dislike new0() to require the type. I'd prefer: font = new0(font); But this is horrible to read, so I went with calloc(1, sizeof(*xyz)); But the systemd code-base seems to use new0() consistently so I can adjust all my calloc()s if you want? I'm fine with keeping consistency here. I'd really prefer that. Not that new() is a glibc thing, where they have g_new(). Of all the type-safe macros for allocation I certainly like it the best... For public APIs we are pretty defensive and check all parameters we take. Given you kinda made this a public library it might make sense to follow that rule here, too. assert_return() is particularly useful for this. (Well, not for the instance above, bug if you return a negative errno it is super-useful). I wanna avoid these in fast-paths (the blending/blitting functions) but for all the other ones, I can add assert()s. Yes, please do not add assert()s in inner loops! I mean, the whole assert() story is not that definitive either. I add them to a lot of functions, but I can totally understand when people think I am adding to many of them Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
On Sun, 01.12.13 06:28, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: On Wed, Nov 27, 2013 at 07:48:40PM +0100, David Herrmann wrote: As a first step, we add the required header+build-chain and add the font-handling. To avoid heavy font-pipelines in systemd, we only provide a statically-sized fallback-font based on GNU-Unifont. Hi David, I don't think that GNU-Unifont is licensed in a way that allows it to be embedded in systemd. Systemd is LGPLv2+, while Unifont is GPLv2+ + FontException. I don't think this is really a problem, is it? LGPL2+ is after all compatible wit GPL2+. When you link stuff that is LGPL2+ with stuff that is GPL2+ you effectively just downgrade LGPL2+ to GPL2+, and get a result that effectively should be treated as GPL2+ altogether. Now, what matters for us is that all our APIs and our sources stay LGPL2+, so that we can freely copy/paste code around and library users can use the stuff with little restrictions. However, if systemd-consoled at runtime is downgraded to GPL2+, then that should not really affect anybody... I mean, if somebody wanted to link non-free code into consoled, then we'd have a problem, but nobody does that and consoled is not a program where that's even something to think off. I'd be fine to do with unifont what we do with the pci/usb-db, and ship it in the sources, and provide an easy way to update it from upstream (via a make target). Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
On Sun, 01.12.13 10:05, David Herrmann (dh.herrm...@gmail.com) wrote: I'm fine with installing the file into the system, but I doubt we win much. It's meant as fallback for early-boot, initrd and so on. If we keep it separate, we must make sure to include it in any systems we build (initrd, containers, vms, ..). So if there's no reason beside license issues, I'd like to keep it built-in. So if it is acceptable for systemd-gfx *binary* to be GPLv2+ licensed, we could use the system unifont.hex file at build time, and actually link it into the binary. I propose that we try to go this way. That's what I currently do. Or we could have the package also contain the converted font in appropriate format, and mmap it at runtime. But this is more complex, and doesn't actually avoid the licensing issue, since the font would still be GPLv2+. Where is the difference between build-time linking and mmap()? (regarding licensing) Also, where's the point of keeping libsystemd-gfx.so LGPL just to have a *mandatory* dependency which is GPL? I think embedding the thing into our binary in question is the best choice here. I don't see any license problems, and it's certainly the fastest and most robust thing to do... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
On Wed, Dec 11, 2013 at 1:56 AM, Lennart Poettering lenn...@poettering.net wrote: On Sun, 01.12.13 10:05, David Herrmann (dh.herrm...@gmail.com) wrote: I'm fine with installing the file into the system, but I doubt we win much. It's meant as fallback for early-boot, initrd and so on. If we keep it separate, we must make sure to include it in any systems we build (initrd, containers, vms, ..). So if there's no reason beside license issues, I'd like to keep it built-in. So if it is acceptable for systemd-gfx *binary* to be GPLv2+ licensed, we could use the system unifont.hex file at build time, and actually link it into the binary. I propose that we try to go this way. That's what I currently do. Or we could have the package also contain the converted font in appropriate format, and mmap it at runtime. But this is more complex, and doesn't actually avoid the licensing issue, since the font would still be GPLv2+. Where is the difference between build-time linking and mmap()? (regarding licensing) Also, where's the point of keeping libsystemd-gfx.so LGPL just to have a *mandatory* dependency which is GPL? I think embedding the thing into our binary in question is the best choice here. I don't see any license problems, and it's certainly the fastest and most robust thing to do... It is huge, ~2MB. We need to be sure that we will never have 2 different processes carrying it, it would be a waste of memory. Storing it on disk and mmap()ing it from multiple different binaries would be a fine alternative then, I think. Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
On Wed, 11.12.13 02:14, Kay Sievers (k...@vrfy.org) wrote: On Wed, Dec 11, 2013 at 1:56 AM, Lennart Poettering lenn...@poettering.net wrote: On Sun, 01.12.13 10:05, David Herrmann (dh.herrm...@gmail.com) wrote: I'm fine with installing the file into the system, but I doubt we win much. It's meant as fallback for early-boot, initrd and so on. If we keep it separate, we must make sure to include it in any systems we build (initrd, containers, vms, ..). So if there's no reason beside license issues, I'd like to keep it built-in. So if it is acceptable for systemd-gfx *binary* to be GPLv2+ licensed, we could use the system unifont.hex file at build time, and actually link it into the binary. I propose that we try to go this way. That's what I currently do. Or we could have the package also contain the converted font in appropriate format, and mmap it at runtime. But this is more complex, and doesn't actually avoid the licensing issue, since the font would still be GPLv2+. Where is the difference between build-time linking and mmap()? (regarding licensing) Also, where's the point of keeping libsystemd-gfx.so LGPL just to have a *mandatory* dependency which is GPL? I think embedding the thing into our binary in question is the best choice here. I don't see any license problems, and it's certainly the fastest and most robust thing to do... It is huge, ~2MB. We need to be sure that we will never have 2 different processes carrying it, it would be a waste of memory. That's a very good point. Given that we link everything in systemd statically so far, then this is a strong point to store in an external mmapable binary file if this is used by more than one process... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
Hi The binary file contains all glyphs in a compressed 1-bit-per-pixel format for all 8x16 and 16x16 glyphs. It is linked directly into libsystemd-gfx via the *.bin makefile target. Binary size is 2.1MB, but thanks to paging, the kernel only loads required pages into memory. A ASCII-only screen thus only needs 40k VIRT mem. Do we have quad-glyphs (16x32) for the ASCII part of this for hi-res screens? (also, if we use NFD we could reuse these for some of unicode[1], but that is less important) [1]http://blog.golang.org/normalization See here: +static void gfx_glyph_blend(gfx_glyph *g, unsigned int ppi, const struct unifont_data *u) { +unsigned int i, j; +const uint8_t *src; +uint8_t *dst; + +/* + * TODO: scale glyph according to @ppi + */ Idea is to simply scale the glyph by an integer (sth like max(1, ppi / 72)). This will result in a readable font-size on all screens. Note that we don't care for hi-res fonts, we're not EFI-firmware.. Regarding NFD: I don't care for normalization. I don't do any string-comparisons. We support combining characters in the font-renderer just fine, so I don't think we ever need unicode normalization. Thanks David ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
Hi On Sun, Dec 1, 2013 at 6:28 AM, Zbigniew Jędrzejewski-Szmek zbys...@in.waw.pl wrote: On Wed, Nov 27, 2013 at 07:48:40PM +0100, David Herrmann wrote: As a first step, we add the required header+build-chain and add the font-handling. To avoid heavy font-pipelines in systemd, we only provide a statically-sized fallback-font based on GNU-Unifont. Hi David, I don't think that GNU-Unifont is licensed in a way that allows it to be embedded in systemd. Systemd is LGPLv2+, while Unifont is GPLv2+ + FontException. FontException allows embedding in documents, so it doesn't apply. I disagree. I'm allowed to embed GNU-Unifont in a pdf/postscript file, right? However, postscript is as turing-complete as x86-assembler, so I don't see the difference between an ELF-document and a postscript-document. It would be possible have some sources which are GPLv2+ only, but I think we want to avoid such complications. It's not about sources. Assuming the font-exception doesn't apply, this only means all binaries linking to libsystemd-gfx are GPLv2. The sources stay LGPL as usual. Also, if the font was embedded in systemd, distributions would then remove it in order to replace is with the system version. So I think that including the font sources is pointless... Debian has it packaged [1], but an old version, I'm not sure if there have been recent updates, and possibly in the wrong format. Fedora doesn't seem to have it yet. But adding fonts is easy, I'd do the Fedora package myself, and other distributions could surely add/update it. I'm fine with installing the file into the system, but I doubt we win much. It's meant as fallback for early-boot, initrd and so on. If we keep it separate, we must make sure to include it in any systems we build (initrd, containers, vms, ..). So if there's no reason beside license issues, I'd like to keep it built-in. So if it is acceptable for systemd-gfx *binary* to be GPLv2+ licensed, we could use the system unifont.hex file at build time, and actually link it into the binary. I propose that we try to go this way. That's what I currently do. Or we could have the package also contain the converted font in appropriate format, and mmap it at runtime. But this is more complex, and doesn't actually avoid the licensing issue, since the font would still be GPLv2+. Where is the difference between build-time linking and mmap()? (regarding licensing) Also, where's the point of keeping libsystemd-gfx.so LGPL just to have a *mandatory* dependency which is GPL? Thanks David ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
On Sat, Nov 30, 2013 at 9:28 PM, Zbigniew Jędrzejewski-Szmek zbys...@in.waw.pl wrote: On Wed, Nov 27, 2013 at 07:48:40PM +0100, David Herrmann wrote: As a first step, we add the required header+build-chain and add the font-handling. To avoid heavy font-pipelines in systemd, we only provide a statically-sized fallback-font based on GNU-Unifont. Hi David, I don't think that GNU-Unifont is licensed in a way that allows it to be embedded in systemd. Systemd is LGPLv2+, while Unifont is GPLv2+ + FontException. FontException allows embedding in documents, so it doesn't apply. It would be possible have some sources which are GPLv2+ only, but I think we want to avoid such complications. Also, if the font was embedded in systemd, distributions would then remove it in order to replace is with the system version. So I think that including the font sources is pointless... Debian has it packaged [1], but an old version, I'm not sure if there have been recent updates, and possibly in the wrong format. Fedora doesn't seem to have it yet. But adding fonts is easy, I'd do the Fedora package myself, and other distributions could surely add/update it. So if it is acceptable for systemd-gfx *binary* to be GPLv2+ licensed, we could use the system unifont.hex file at build time, and actually link it into the binary. I propose that we try to go this way. Or we could have the package also contain the converted font in appropriate format, and mmap it at runtime. But this is more complex, and doesn't actually avoid the licensing issue, since the font would still be GPLv2+. The font is needed in the initramfs Zbyszek [1] http://packages.debian.org/sid/all/unifont/filelist ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
Hi On Wed, Nov 27, 2013 at 11:11 PM, Lennart Poettering lenn...@poettering.net wrote: On Wed, 27.11.13 19:48, David Herrmann (dh.herrm...@gmail.com) wrote: This patch introduces sd-gfx, a systemd-internal library dealing with all these things. Note that it is designed to be exported some day, but for now we keep it internal. While the library itself will be kept small and almost self-contained, it is designed to be extensible and can be used in rather complex graphics applications. For systemd, we plan to add a basic emergency-console, an initrd password-query, kernel-log-screen and a fallback login-screen. These allow booting without CONFIG_VT and provide a basic system for seats without VTs (either CONFIT_VT=n or seats != seat0). BTW, one more use of this I'd like to see: if we boot-up and detect that no AC and very low battery we should show a pretty screen and block the boot until AC is plugged in or the user hits some magic key... Yepp, sounds good. Would be like 200 lines to write that, I guess. +static int gfx_glyph_render(gfx_glyph *g, unsigned int ppi, const struct unifont_data *u) { +g-buf.width = u-cells * unifont_width; +g-buf.height = unifont_height; +g-buf.stride = u-cells * unifont_stride; +g-buf.format = SD_GFX_BUFFER_FORMAT_A1; + +g-buf.data = calloc(unifont_height, unifont_max_cells * unifont_stride); +if (!g-buf.data) +return -ENOMEM; Nice, this must be the first use of alloc() I have ever seen that actually takes benefit of the weird parameters it takes ;-) Do I get an award or something? :) + +gfx_glyph_blend(g, ppi, u); +return 0; +} + +int sd_gfx_font_new(sd_gfx_font **out, unsigned int ppi) { Hmm, so far we followed the rough rule that parameters we pass out are listed last on the function parameter list. int sd_gfx_font_new(unsigned ppi, sd_gfx_fond **out); +sd_gfx_font *font; +int r; + +ppi = CLAMP(ppi, 10U, 1000U); + +font = calloc(1, sizeof(*font)); font = new0(sd_gfx_font, 1); I actually dislike new0() to require the type. I'd prefer: font = new0(font); But this is horrible to read, so I went with calloc(1, sizeof(*xyz)); But the systemd code-base seems to use new0() consistently so I can adjust all my calloc()s if you want? I'm fine with keeping consistency here. +font-glyphs = hashmap_new(trivial_hash_func, trivial_compare_func); +if (!font-glyphs) { +free(font); +return log_oom(); Hmm, library code should never log. Please just return -ENOMEM here. log_oom() should be used in main programs only really... +void sd_gfx_font_free(sd_gfx_font *font) { +gfx_glyph *g; For public APIs we are pretty defensive and check all parameters we take. Given you kinda made this a public library it might make sense to follow that rule here, too. assert_return() is particularly useful for this. (Well, not for the instance above, bug if you return a negative errno it is super-useful). I wanna avoid these in fast-paths (the blending/blitting functions) but for all the other ones, I can add assert()s. Most destructors we have tend to accept NULL pointers too. It makes clean-up code a lot easier (especially in combination with gcc cleanup attributes). libc free() takes NULL pointers too, so this is a natural extension. Oh, I know. This is presumably the only destructor where I somehow forgot that. Fixed. +void sd_gfx_font_render(sd_gfx_font *font, uint32_t id, const uint32_t *ucs4, size_t len, sd_gfx_buffer **out) { +const struct unifont_data *u; +gfx_glyph *g; + +if (!len) +goto error; + +if (!id) +id = *ucs4; + +g = hashmap_get(font-glyphs, INT_TO_PTR(id)); +if (g) +goto out; + +g = calloc(1, sizeof(*g)); g = new(gfx_glyph, 1); +if (!g) { +log_oom(); +goto error; +} + +u = unifont_get(*ucs4); +if (gfx_glyph_render(g, font-ppi, u) 0) { +log_oom(); +free(g); +goto error; +} + +while (--len) { +u = unifont_get(*++ucs4); +gfx_glyph_blend(g, font-ppi, u); +} + +if (hashmap_put(font-glyphs, INT_TO_PTR(id), g) 0) { +log_oom(); +free(g-buf.data); +free(g); +goto error; +} + +goto out; + +error: +g = font-fallback; +out: +*out = g-buf; +} Hmm, we so far followed to rule to not clobber return parameters on failure. i.e. we try hard to fill any return values in until we know that nothing can fail anymore. This makes the contract for the caller much easier who can refrain from freeing/resetting any parameters if a call failed...
[systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
If we want to interact with a user in the initrd, during emergency-situations, in single-user mode, or in any other rather limited situation, we currently rely on the kernel to do input and graphics access for us. More precisely, we rely on the VT layer to do keyboard parsing and font rendering. Or in other words: The *kernel* provides our UI! This is bad for the same reasons we don't put any other UI into the kernel. However, there are a few reasons to keep a minimal UI in the kernel: 1) show panic-screen with kernel oops message 2) show log-screen during early boot 3) allow kernel-debugging via kdb While people using kdb are encouraged to keep VTs, for 1) and 2) there is a replacement via fblog/drmlog. So we run out of reasons to keep a UI in the kernel. However, to allow moving the UI handling to userspace, we need a bunch of helpers: - keyboard handling: convert keycodes into keysyms and modifiers/.. - modesetting: program the gfx pipeline and provide a rendering infrastructure - fonts: to render text, we need some basic fonts - hotplugging: device detection and assignment during runtime (Note that all these are implemented (often quite rudimentary) in the kernel to allow a basic UI.) This patch introduces sd-gfx, a systemd-internal library dealing with all these things. Note that it is designed to be exported some day, but for now we keep it internal. While the library itself will be kept small and almost self-contained, it is designed to be extensible and can be used in rather complex graphics applications. For systemd, we plan to add a basic emergency-console, an initrd password-query, kernel-log-screen and a fallback login-screen. These allow booting without CONFIG_VT and provide a basic system for seats without VTs (either CONFIT_VT=n or seats != seat0). As a first step, we add the required header+build-chain and add the font-handling. To avoid heavy font-pipelines in systemd, we only provide a statically-sized fallback-font based on GNU-Unifont. It contains glyphs for the *whole* Base-Multilingual-Plane of Unicode and thus allows internationalized text. The make-unifont.py script is used by the make update-unifont custom target to regenerate the unifont files. As this can take quite some time, we check the result into git so only maintainers need to run this. The binary file contains all glyphs in a compressed 1-bit-per-pixel format for all 8x16 and 16x16 glyphs. It is linked directly into libsystemd-gfx via the *.bin makefile target. Binary size is 2.1MB, but thanks to paging, the kernel only loads required pages into memory. A ASCII-only screen thus only needs 40k VIRT mem. --- Hi I removed the unifont-data from this patch so this will not apply cleanly. However, the ML would reject the huge patch otherwise. If anyone is interested in the raw patch, the series is available on: http://cgit.freedesktop.org/~dvdhrm/systemd/log/?h=console Thanks David Makefile.am |31 + configure.ac |10 + make-unifont.py | 138 + src/libsystemd-gfx/.gitignore| 1 + src/libsystemd-gfx/Makefile | 1 + src/libsystemd-gfx/gfx-unifont.c | 273 + src/libsystemd-gfx/unifont.bin | Bin 0 - 2162688 bytes src/libsystemd-gfx/unifont.hex | 63488 + src/systemd/sd-gfx.h |65 + 9 files changed, 64007 insertions(+) create mode 100755 make-unifont.py create mode 100644 src/libsystemd-gfx/.gitignore create mode 12 src/libsystemd-gfx/Makefile create mode 100644 src/libsystemd-gfx/gfx-unifont.c create mode 100644 src/libsystemd-gfx/unifont.bin create mode 100644 src/libsystemd-gfx/unifont.hex create mode 100644 src/systemd/sd-gfx.h diff --git a/Makefile.am b/Makefile.am index ce27e82..2edb091 100644 --- a/Makefile.am +++ b/Makefile.am @@ -3853,6 +3853,37 @@ EXTRA_DIST += \ endif # -- +if HAVE_GFX +noinst_LTLIBRARIES += \ + libsystemd-gfx.la + +libsystemd_gfx_la_SOURCES = \ + src/libsystemd-gfx/sd-gfx.h \ + src/libsystemd-gfx/gfx-unifont.c + +libsystemd_gfx_la_CFLAGS = \ + $(AM_CFLAGS) + +libsystemd_gfx_la_LIBADD = \ + libsystemd-shared.la \ + src/libsystemd-gfx/unifont.bin.lo + +src/libsystemd-gfx/unifont.bin: make-unifont.py src/libsystemd-gfx/unifont.hex + $(AM_V_GEN)cat $(top_srcdir)/src/libsystemd-gfx/unifont.hex | $(PYTHON) $ $@ + +src/libsystemd-gfx/unifont.cmp: make-unifont.py src/libsystemd-gfx/unifont.bin + $(AM_V_GEN)cat $(top_srcdir)/src/libsystemd-gfx/unifont.bin | $(PYTHON) $ verify $@ + +update-unifont: src/libsystemd-gfx/unifont.bin src/libsystemd-gfx/unifont.cmp + @RET=`diff -u src/libsystemd-gfx/unifont.hex src/libsystemd-gfx/unifont.cmp | wc -l` ; \ + if test x$$? != x0 -o x$$RET != x0 ; then \ + echo Generated Unifont-file differs from original;
Re: [systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section
On Wed, 27.11.13 19:48, David Herrmann (dh.herrm...@gmail.com) wrote: This patch introduces sd-gfx, a systemd-internal library dealing with all these things. Note that it is designed to be exported some day, but for now we keep it internal. While the library itself will be kept small and almost self-contained, it is designed to be extensible and can be used in rather complex graphics applications. For systemd, we plan to add a basic emergency-console, an initrd password-query, kernel-log-screen and a fallback login-screen. These allow booting without CONFIG_VT and provide a basic system for seats without VTs (either CONFIT_VT=n or seats != seat0). BTW, one more use of this I'd like to see: if we boot-up and detect that no AC and very low battery we should show a pretty screen and block the boot until AC is plugged in or the user hits some magic key... +static int gfx_glyph_render(gfx_glyph *g, unsigned int ppi, const struct unifont_data *u) { +g-buf.width = u-cells * unifont_width; +g-buf.height = unifont_height; +g-buf.stride = u-cells * unifont_stride; +g-buf.format = SD_GFX_BUFFER_FORMAT_A1; + +g-buf.data = calloc(unifont_height, unifont_max_cells * unifont_stride); +if (!g-buf.data) +return -ENOMEM; Nice, this must be the first use of alloc() I have ever seen that actually takes benefit of the weird parameters it takes ;-) + +gfx_glyph_blend(g, ppi, u); +return 0; +} + +int sd_gfx_font_new(sd_gfx_font **out, unsigned int ppi) { Hmm, so far we followed the rough rule that parameters we pass out are listed last on the function parameter list. int sd_gfx_font_new(unsigned ppi, sd_gfx_fond **out); +sd_gfx_font *font; +int r; + +ppi = CLAMP(ppi, 10U, 1000U); + +font = calloc(1, sizeof(*font)); font = new0(sd_gfx_font, 1); +font-glyphs = hashmap_new(trivial_hash_func, trivial_compare_func); +if (!font-glyphs) { +free(font); +return log_oom(); Hmm, library code should never log. Please just return -ENOMEM here. log_oom() should be used in main programs only really... +void sd_gfx_font_free(sd_gfx_font *font) { +gfx_glyph *g; For public APIs we are pretty defensive and check all parameters we take. Given you kinda made this a public library it might make sense to follow that rule here, too. assert_return() is particularly useful for this. (Well, not for the instance above, bug if you return a negative errno it is super-useful). Most destructors we have tend to accept NULL pointers too. It makes clean-up code a lot easier (especially in combination with gcc cleanup attributes). libc free() takes NULL pointers too, so this is a natural extension. +void sd_gfx_font_render(sd_gfx_font *font, uint32_t id, const uint32_t *ucs4, size_t len, sd_gfx_buffer **out) { +const struct unifont_data *u; +gfx_glyph *g; + +if (!len) +goto error; + +if (!id) +id = *ucs4; + +g = hashmap_get(font-glyphs, INT_TO_PTR(id)); +if (g) +goto out; + +g = calloc(1, sizeof(*g)); g = new(gfx_glyph, 1); +if (!g) { +log_oom(); +goto error; +} + +u = unifont_get(*ucs4); +if (gfx_glyph_render(g, font-ppi, u) 0) { +log_oom(); +free(g); +goto error; +} + +while (--len) { +u = unifont_get(*++ucs4); +gfx_glyph_blend(g, font-ppi, u); +} + +if (hashmap_put(font-glyphs, INT_TO_PTR(id), g) 0) { +log_oom(); +free(g-buf.data); +free(g); +goto error; +} + +goto out; + +error: +g = font-fallback; +out: +*out = g-buf; +} Hmm, we so far followed to rule to not clobber return parameters on failure. i.e. we try hard to fill any return values in until we know that nothing can fail anymore. This makes the contract for the caller much easier who can refrain from freeing/resetting any parameters if a call failed... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel