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... Oh, I don't clobber the return value here. This is a "void" function, it never fails. The error-path just stores a fallback-glyph in the out-buffer. This way, you get some weird "this-is-an-unprintable-or-caused-by-error"-glyph instead of.. nothing. The application can assume glyph-rendering always works and just don't deal with possible errors here. I actually rely quite often on "out" parameters to not get touched at all if a function fails. It makes error-paths so much nicer. Thanks David _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel