Debugging critical warnings from GLib code

tl;dr: G_DEBUG=fatal-warnings gdb ./my-program

If you have some code which uses GLib, and it emits a critical warning, for example if a g_return_if_fail() check fails or if a g_warning() message is emitted, how do you track it down and debug it?

Run your code under gdb with G_DEBUG=fatal-warnings (for g_return_if_fail() and g_warning()) or G_DEBUG=fatal-criticals (for g_return_if_fail()), and gdb will break execution when the failing precondition or warning is reached. If there are multiple warnings and you want to skip through to get to a particular one, just use the continue command in gdb until you reach the one you want.

Cave exploration in Austria

For once, this is going to be a non-technical post. I hope to share some of what I’ve been up to in my summer holidays this year.

In late July, I spent two weeks on the Löser plateau in Austria, as part of a long-running caving expedition exploring the caves up there. The plateau is a huge expanse of limestone, opposite the Dachstein, and it contains hundreds of caves of varying sizes. The same expedition has been going there every summer for the last 40 years, slowly working its way across the plateau, trying to find big and deep caves. This was the first time I’d joined them.

Credit: Chris Densham

Some brief background: What is caving? It’s a sport where people descend caves, generally to the bottom (or as deep as they can get), to see and map what is there. It typically involves a lot of water (less of that in Austria than the UK) and mud, cold temperatures (definitely cold in Austria), and technical rope work to descend and ascend vertical shafts (‘pitches’). It combines the skills of climbing, scrambling and surveying; and often requires unshakeable enthusiasm for prolonged physical misery. It’s good fun.

Credit: Luke Stangroom

This year, we focused primarily on two existing (and large) caves: Tunnock’s, and Balcony. I spent a number of my days down Balcony, at around -300m (that’s 300m vertically below the entrance of the cave). We explored various new bits of passage, including a 100m×80m×80m chamber which, sadly, was a dead end; but good fun to get to and explore. Other trips included setting up the ropes (’rigging’) in some bits of cave so they could be re-explored from previous years; and re-surveying some other pieces of passage where the original surveys were incorrect.

Credit: Brendan Hall

Aside from trips down Balcony, we spent some time prospecting for new caves, finding a couple of promising new ones, and another which looked promising then turned into a dead end after 100m of depth. Since I left, another few cave entrances (some new, some rediscovered from 2012) have been found, and leads have been pushed even further in the existing caves.

Credit: Brendan Hall

What are conditions like in the caves? Unlike caves in the UK, most of the ones on the Löser plateau are dry apart from one or two sections. It’s only very recently that exploration has got down to a depth which routinely sees water. There is some mud, but not as much as in the UK. However, what there is is thicker and more pervasive. There’s generally more sand than one sees in caves in the UK, which does a good job of gritting up equipment and hands (think about what happens whenever you go to the beach). The caves are cold, but not ludicrously so — a few of them are cold enough to maintain large ice columns, but I was warm enough in my UK caving gear without extra thermals.

Credit: Brendan Hall

Why do people do this expedition caving? Many reasons, but most commonly, because it takes you to interesting new places, it’s a technical challenge, it’s a physical challenge, and the other people who do it are good fun to be around.

Credit: Luke Stangroom

When not caving, due to tiredness, laziness or weather, people spent their time in the valley, relaxing and drawing up surveys of the sections of caves they’d recently explored (‘nerding’). There are various bits of software for this, which take the legs of dead-reckoned survey and tie them together, using error distribution through loop closures to increase accuracy. The results are pretty nifty, though it takes a while to get up to speed with the software and draw up your surveys efficiently.

Credit: http://expo.survex.com/1623/264/264.html

Caving’s a fun sport with opportunities to go places where literally no human has ever been before, if you take it far enough. It’s easy to get into, too. Read more updates from the expedition if you want.

GUADEC 2017: sun, rain, Coverity, walks

GUADEC 2017 has ended in Manchester. It’s been great; thanks to the organisers and sponsors for a fun conference (this year’s highlight: a preponderance of Tiki bars).

We’ve had sun and heat, and we’ve had rain and more rain. Often within the same hour. On the final day of the conference, a group of us went out to Edale to do some walks to see the Peak District, a national park area near Manchester. This is an area I’ve visited many times before, so it was fun to be able to show it to GNOME people.

This year I gave a talk about the Coverity scans I’ve been running on various GNOME and freedesktop modules for the last year. The slides are online and the video will be up with the rest of the GUADEC videos. If you have a security-critical (or other) module which you’d like to be included in the scan set, let me know. Coverity’s good at finding bugs in complex control flows, but you do need to put some time into triaging its reports. I’m happy to provide guidance about using it.

I spent a fair amount of time during the unconference days reviewing Simon McVittie’s D-Bus work to add support for app-containers into the D-Bus specification and dbus-daemon. This is the first part of an effort to improve support for exposing unconfined D-Bus services to confined app-containers safely and efficiently. The rest of my time was spent working on exciting support for updating flatpak over the LAN for Endless OS. I’ll blog about this more in future.

Thanks to the GUADEC team for organising a great conference, the conference sponsors, and to my employer, Endless, for sponsoring me to go.

Building a GNOME nightly app: Hitori

Following on from earlier efforts to make Hitori a flatpak app, it’s now available as a GNOME nightly app (click here to install), built from git master.

Thanks to hard work by Alex Larsson and others, this was ridiculously easy (see the wiki page on it):

  1. Write flatpak manifest with source and build instructions for Hitori (test locally with flatpak-builder)
  2. Add .app file pointing to it in gnome-apps-nightly
  3. Wait for build to complete
  4. Add .flatpakref file pointing to the build in gnome-apps-nightly

Speaking of Hitori, I don’t have much time to maintain it at the moment, and there are some interesting open feature requests. If anybody is looking for a fun little project to take on, I am happy to mentor work on them.

Running GitLab CI on autotools projects

Inspired by the talk at FOSDEM, I’ve just enabled GitLab’s continuous integration (CI) for building make distcheck for Walbottle, and it was delightfully easy. The results are on Walbottle’s GitLab page.

Steps

  1. Create a ci branch to contain the mess you’ll make while iterating over the correct compile steps.
  2. Create and push a .gitlab-ci.yml file containing build rules similar to the following:
    image: debian:unstable
    
    before_script:
      - apt update -qq
      - apt install -y -qq build-essential autoconf automake pkg-config libtool m4 autoconf-archive gtk-doc-tools libxml2-utils gobject-introspection libgirepository1.0-dev libglib2.0-dev libjson-glib-dev
    
    stages:
      - build
    
    # FIXME: Re-enable valgrind once running the tests under it doesn’t take forever (it causes timeouts).
    # Re-add valgrind to apt-install line above
    build-distcheck:
      stage: build
      script:
        - mkdir build
        - cd build
        - ../autogen.sh --disable-valgrind
        - make V=1 VERBOSE=1
        - DISTCHECK_CONFIGURE_FLAGS=--disable-valgrind make distcheck V=1 VERBOSE=1
    
      # The files which are to be made available in GitLab
      artifacts:
        paths:
          - build/*
  3. Iterate a few times until you get all the dependencies right.
  4. Fix any problems you find (because this might well find problems with your dependency declaration in configure.ac, or other distcheck problems in your project).
  5. Merge ci to master and profit from CI results on every branch and master commit.

Looking at the .gitlab-ci.yml file

For information on the overall layout of the YAML file, and the phases available, you’re best off looking at the comprehensive GitLab documentation. Here are some notes about the autotools-and-C–specific bits of it:

  • The image is a Docker image; I picked a Debian one from the Docker hub.
  • Package installation seems to need to be done in the before_script phase, or the packages can’t be found (which is presumably a protection against rogue build systems).
  • I chose to build distcheck in my build rule because that runs the build, runs the tests, and tries various srcdir ? builddir configurations. You can add other build targets (like build-distcheck to try other build setups).
  • Pass V=1 VERBOSE=1 to get verbose build and test log output in your CI build logs, otherwise you will struggle to work out what is causing any failures.
  • Note that configure flags passed to ./configure are not automatically passed in again when ./configure is run as part of distcheck — so use DISTCHECK_CONFIGURE_FLAGS for that. Ideally, your project will be less fragile than mine, and hence not need any of this.
  • Export the whole build directory as an artifact on success, so you can look at any of the build objects, or the generated tarball, or documentation. You could limit this (for example, to just the tarball) if you’re sure you’ll never need the rest of it.

Going to FOSDEM

I’m going to FOSDEM 2017!

I’ll have a spare, unopened, Nitrokey Pro with me to give to anyone who’s got a good plan for improving the user experience for them in GNOME. That might mean making the setup seamless; it might mean working on the rewrite of Seahorse; it might mean integrating them with LUKS; or something else. Contact me if you’re interested and have a plan.

Validating e-mail addresses

tl;dr: Most likely, you want to validate using the regular expression from the WhatWG (please think about the trade-off you want between practicality and precision); but if you read the caveats below and still want to validate to RFC 5322, then you want libemailvalidation.

Validating e-mail addresses is hard, and not something which you normally want to do in great detail: while it’s possible to spend a lot of time checking the syntax of an e-mail address, the real measure of whether it’s valid is whether the mail server on that domain accepts it. There is ultimately no way around checking that.

Given that a lot of mail providers implement their own restrictions on the local-part (the bit before the ‘@’) of an e-mail address, an address like !!@gmail.com (which is syntactically valid) probably won’t actually be accepted. So what’s the value in doing syntax checks on e-mail addresses? The value is in catching trivial user mistakes, like pasting the wrong data into an e-mail address field, or making a trivial typo in one.

So, for most use cases, there’s no need to bother with fancy validation: just check that the e-mail address matches the regular expression from the WhatWG. That should catch simple mistakes, accept all valid e-mail addresses, and reject some invalid addresses.

Why have I been doing further? Walbottle needs it — I think where one RFC references another is one of the few times it’s necessary to fully implement e-mail validation. In this case, Walbottle needs to be able to validate e-mail addresses provided in JSON files, for its email defined format.

So, I’ve just finished writing a small copylib to validate e-mail addresses according to all the RFCs I could get my hands on; mostly RFC 5322, but there is a sprinking of 5234, 5321, 3629 and 6532 in there too. It’s called libemailvalidation (because naming is hard; typing is easier). Since it’s only about 1000 lines of code, there seems to be little point in building a shared library for it and distributing that; so add it as a git submodule to your code, and use validate.c and validate.h directly. It provides a single function:

size_t error_position;

is_valid = emv_validate_email_address (address_to_check,
                                       length_of_address_to_check,
                                       EMV_VALIDATE_FLAGS_NONE,
                                       &error_position);

if (!is_valid)
  fprintf (stderr, "Invalid e-mail address; error at byte %zu\n",
           error_position);

I’ve had fun testing this lot using test cases generated from the ABNF rules taken directly from the RFCs, thanks to abnfgen. If you find any problems, please get in touch!

Fun fact for the day: due to the obs-qp rule, a valid e-mail address can contain a nul byte. So unless you ignore deprecated syntax for e-mail addresses (not an option for programs which need to be interoperable), e-mail addresses cannot be passed around as nul-terminated strings.

Where are messages on the terminal coming from?

From a discussion on #gtk+ this morning: if you’re using recent versions of GLib with structured logging support, and you want to work out which bit of your code is causing a certain message to be printed to the terminal, run your application in gdb and add a breakpoint on g_log_writer_standard_streams.

(This assumes you’re using the default log writer function; if not, you need to add a breakpoint on something in your writer function.)

Thoughts about reviewing large patchsets

I have recently been involved in reviewing some large feature patchsets for a project at work, and thought it might be interesting to discuss some of the principles we have been trying to stick to when going about these reviews.

These are just suggestions which will not apply verbatim to every project, but they might provoke some ideas which are relevant to your project. In many cases, a developer might find a checklist is not useful for them — it is too rigid, or slows down the pace of development too much. Checklists are probably best treated as strong suggestions, rather than strict requirements for a review to pass; developers need to use them as reminders for things they should think about when submitting a patch, rather than a box-ticking exercise. Checklists probably work better in larger projects with lots of infrequent or inexperienced contributors, who may not be aware of all of the conventions of the project.

Overall principles

In order to break a set of reviews down into chunks, there a few key principles to stick to:

  • Review patches which are as small as possible, but no smaller (see here, here and here)
  • Learn from each review so the same review comments do not need to be made more than once
  • Use automated tools to eliminate many of the repetitive and time consuming parts of patch review and rework
  • Do high-level API review first, ideally before implementing those APIs, to avoid unnecessary rework

Pre-submission checklist

(A rationale for each of these points is given in the section below to avoid cluttering this one.)

This is the pre-submission checklist we have been using to check patches against before submission for review:

  1. All new code follows the coding guidelines, especially the namespacing guidelines, memory management guidelines, pre- and post-condition guidelines, and introspection guidelines — some key points from these are pulled out below, but these are not the only points to pay attention to.
  2. All new public API must be namespaced correctly.
  3. All new public API must have a complete and useful documentation comment.
  4. All new public API documentation comments must have GObject Introspection annotations where appropriate; g-ir-scanner (part of the build process) must emit no warnings when run with --warn-all --warn-error (which should be set by $(WARN_SCANNERFLAGS) from AX_COMPILER_FLAGS).
  5. All new public methods must have pre- and post-conditions to enforce constraints on the accepted parameter values.
  6. The code must compile without warnings, after ensuring that AX_COMPILER_FLAGS is used and enabled in configure.ac (if it is correctly enabled, compiling the module should fail if there are any compiler warnings) — remember to add $(WARN_CFLAGS), $(WARN_LDFLAGS) and $(WARN_SCANNERFLAGS) to new Makefile.am targets as appropriate.
  7. The introduction documentation comment for each new object must give a usage example for each of the main ways that object is intended to be used.
  8. All new code must be formatted as per the project’s coding guidelines.
  9. If possible, there should be an example program for each new feature or object, which can be used to manually test that functionality — these examples may be submitted in a separate patch from the object implementation, but must be submitted at the same time as the implementation in order to allow review in parallel. Example programs must be usable when installed or uninstalled, so they can be used during development and on production machines.
  10. There must be automated tests (using the GTest framework in GLib) for construction of each new object, and for getting and setting each of its properties.
  11. The code coverage of the automated tests must be checked (using make check-code-coverage and AX_CODE_COVERAGE) before submission, and if it’s possible to add more automated tests (and for them to be reliable) to improve the coverage, this should be done; the final code coverage figure for the object should be mentioned in a comment on the patch review, and it would be helpful to have the lcov reports for the object saved somewhere for analysis as part of the review.
  12. There must be no definite memory leaks reported by Valgrind when running the automated tests under it (using AX_VALGRIND_CHECK and make check-valgrind).
  13. All automated tests must be installed as installed-tests so they can be run as integration tests on a production system.
  14. make distcheck must pass before submission of any patch, especially if it touches the build system.
  15. All new code has been checked to ensure it doesn’t contradict review comments from previous reviews of other patches (i.e. we want to avoid making the same review comments on every submitted patch).
  16. Commit messages must explain why they make the changes they do.

Rationales

  1. Each coding guideline has its own rationale for why it’s useful, and many of them significantly affect the structure of a patch, so are important to get right early on.
  2. Namespacing is important for the correct functioning of a lot of the developer tools (for example, GObject Introspection), and to avoid symbol collisions between libraries — checking it is a very mechanical process which it is best to not have to spend review time on.
  3. Documentation comments are useful to both the reviewer and to end users of the API — for the reviewer, they act as an explanation of why a particular API is necessary, how it is meant to be used, and can provide insight into implementation choices. These are questions which the reviewer would otherwise have to ask in the review, so writing them up lucidly in a documentation comment saves time in the long run.
  4. GObject Introspection annotations are a requirement for the platform’s language bindings (to JavaScript or Python, for example) to work, so must be added at some point. Fixing the error messages from g-ir-scanner is sufficient to ensure that the API can be introspected.
  5. Pre- and post-conditions are a form of assertion in the code, which check for programmer errors at runtime. If they are used consistently throughout the code on every API entry point, they can catch programmer errors much nearer their origin than otherwise, which speeds up debugging both during development of the library, and when end users are using the public APIs. They also act as a loose form of documentation of what each API will allow as its inputs and outputs, which helps review (see the comments about documentation above).
  6. The set of compiler warnings enabled by AX_COMPILER_FLAGS have been chosen to balance false positives against false negatives in detecting bugs in the code. Each compiler warning typically identifies a single bug in the code which would otherwise have to be fixed later in the life of the library — fixing bugs later is always more expensive in terms of debugging time.
  7. Usage examples are another form of documentation (as discussed above), which specifically make it clearer to a reviewer how a particular feature is intended to be used. In writing usage examples, the author of a patch can often notice awkwardnesses in their API design, which can then be fixed before review — this is faster than them being caught in review and sent back for modification.
  8. Well formatted code is a lot easier to read and review than poorly formatted code. It allows the reviewer to think about the function of the code they are reviewing, rather than (for example) which function call a given argument actually applies to, or which block of code a statement is actually part of.
  9. Example programs are a loose form of testing, and also act as usage examples and documentation for the feature (see above). They provide an easy way for the reviewer to test a feature, especially if it affects a UI or has some interactive element; this is very hard to do by simply looking at the code in a patch. Their biggest benefit will be when the feature is modified in future — the example programs can be used to test changes to the feature and ensure that its behaviour changes (or does not) as expected.
  10. For each unit test for a piece of code, the behaviour checked by that unit test can be guaranteed to be unchanged across modifications to the code in future. This prevents regressions (especially if the unit tests for the project are set up to be run automatically on each commit by a continuous integration system). The value of unit tests when initially implementing a feature is in the way they guide API design to be testable in the first place. It is often the case that an API will be written without unit tests, and later someone will try to add unit tests and find that the API is untestable; typically because it relies on internal state which the test harness cannot affect. By that point, the API is stable and cannot be changed to allow testing.
  11. Looking at code coverage reports is a good way to check that unit tests are actually checking what they are expected to check about the code. Code coverage provides a simple, coarse-grained metric of code quality — the quality of untested code is unknown.
  12. Every memory leak is a bug, and hence needs to be fixed at some point. Checking for memory leaks in a code review is a very mechanical, time-consuming process. If memory leaks can be detected automatically, by using valgrind on the unit tests, this reduces the amount of time needed to catch them during review. This is an area where higher code coverage provides immediate benefits. Another way to avoid leaks is to use g_autoptr() to automatically free memory when leaving a control block.
  13. If all automated tests are available, they can be run as part of system-wide integration tests, to check that the project behaviour doesn’t change when other system libraries (its dependencies) are changed. This is one of the motivations behind installed-tests. This is a one-time setup needed for your project, and once it’s set up, does not need to be done for each commit.
  14. make distcheck ensures that a tarball can be created successfully from the code, which entails building it, running all the unit tests, and checking that examples compile.
  15. If each patch is updated to learn from the results of previous patch reviews, the amount of time spent making and explaining repeated patch review comments should be significantly reduced, which saves everyone’s time.
  16. Commit messages are a form of documentation of the changes being made to a project. They should explain the motivation behind the changes, and clarify any design decisions which the author thinks the reviewer might question. If a commit message is inadequate, the reviewer is going to ask questions in the review which could have been avoided otherwise.

GTK+ hackfest 2016

A dozen GNOME hackers invaded the Red Hat office in Toronto last week, to spend four days planning the next year of work on our favourite toolkit, GTK+; and to think about how Flatpak applications can best integrate with the rest of the desktop.

What did we do?

  • Worked out an approach for versioning GTK+ in future, to improve the balance between stability and speed of development. This has turned into a wiki page.
  • I demoed Dunfell and added support for visualising GTasks to it. I don’t know how much time I will have for it in the near future, so help and feedback are welcome.
  • There was a detailed discussion of portals for Flatpak, including lots of use cases, and the basics of a security design were decided which allows the most code reuse while also separating functionality. Simon has written more about this.
  • I missed some of the architectural discussion about the future of GTK+ (including moving some classes around, merging some things and stripping out some outdated things), but I believe Benjamin had useful discussions with people about it.
  • Allan, Philip, Mike and I looked at using hotdoc for developer.gnome.org, and possible layouts for a new version of the site. Christian spent some time thinking about integration of documentation into GNOME Builder.
  • Allison did a lot of blogging, and plotted with Alex to add some devious new GVariant functionality to make everyone’s lives easier when writing parsers — I’ll leave her to blog about it.

Thanks to Collabora for sending me along to take part!

After the hackfest, I spent a few days exploring Toronto, and as a result ended up very sunburned.