Hi Ted, Ted Bullock wrote on Mon, Jan 16, 2023 at 12:56:06PM -0700:
> The impetus is that the event(3) manual page isn't all that good for > documenting how to use the library and it is missing functions that are > included in the API and available in the shared library. That seems true, and those are good reasons to improve the manual pages. > Upstream of course has moved on to version 2.x, as far as I can tell > there is no more maintenance being done on the 1.4 version According to https://github.com/libevent/libevent/tree/patches-1.4 that seems accurate to me: it appears the 1.4 branch was last touched in the git repo eight years ago, and it is marked "stale", along with the 2.0 and 2.1 branches. > that OpenBSD ships except the work that is done internally in the OS. Given that our event.3 says .\" Copyright (c) 2000 Artur Grabowski <a...@openbsd.org> and that the libevent git repo contains a Jurassic version of our event.3 in the 1.4 branch and does no longer appear to contain *any* kind of documentation in the master branch - in particular, there is no event.3 in the master branch any longer - i think maintaining the libevent manual pages ourselves is perfectly fine. I see no danger that any documentation might need to be merged from the libevent git: it appears the libevent project regards documentation as a useless distraction at best... :-o > Anyway, here is what I came up with for a few functions after reading > the source tree. I also went back through historical releases to see > how the API evolved over time. I think this is likely a worthwhile project, but still needs a lot of work before it can be considered for commit. > I'm planning to finish off documenting the rest of the library, if > folks have feedback, I'm interested. Given how much feedback i have on your first file, i'm not yet reading your second and third file right now. We do development in steps, and the tree needs to build and be better after each commit than before that commit. So to be able to be committed, your patch for the first file should also delete the now redundant information from event.3. Regard it as splitting one well-defined part out of the excessively large event.3 and adding lots of missing information while splitting it out. > event_init.3 > ============ At this point, one line is missing: .\" $OpenBSD$ The command `mandoc -T lint` tells you: mandoc: event_init.3: STYLE: RCS id missing: (OpenBSD) > .\" Copyright (c) 2023 Ted Bullock <tbull...@comlore.com> > .\" > .\" Permission to use, copy, modify, and distribute this software for any > .\" purpose with or without fee is hereby granted, provided that the above > .\" copyright notice and this permission notice appear in all copies. > .\" > .\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > .\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > .\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > .\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > .\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > .\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > .\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > .\" > .Dd $Mdocdate: January 9 2023 $ > .Dt EVENT_INIT 3 > .Os > .Sh NAME > .Nm event_base_new , > .Nm event_init This is not nice. Unless there is a good reason to name the page after a function that is not the first function in the NAME section, put the function corresponding to the file name first in NAME and in all other sections. > .Nd initializes an > .Vt event_base > instance While nesting markup inside .Nd is technically possible, it is fragile, looks ugly, and we certainly don't do it in OpenBSD. > .Sh SYNOPSIS > .In event.h > .Ft "struct event_base *" > .Fn event_base_new void > .Ft "struct event_base *" > .Fn event_init void > .Sh DESCRIPTION > The functions > .Fn event_base_new > and > .Fn event_init > initialize the > .Lb libevent We don't normally use the .Lb macro in OpenBSD. Maybe just talk about "the event library" with no markup? The existing event.3 talks about The .Nm event API ... which is not wrong, but i'm not convinced the .Nm markup provides much value here. > and need to be called prior to scheduling an event or > starting the event-loop. The two functions are similar, although global > variables are set when calling > .Fn event_init > to help simplify the API for programs requiring only one event-loop. This is not good. Don't start comparing functions without precisely saying first what each function actually does, in particular not in the first sentence of a manual page. Instead, the first sentence should only state the general purpose of the functions documented in the page. In this page, it might not be needed at all, you could probably start with event_base_new(3) right away. After the introductory sentence(s), if any, start with one function and document it thoroughly. After that, document the next function. If they are very similar, it is sometimes useful to document the second one relative to the first one, stating the differences, in particular if the functions do a lot of complicated things, are very similar, the difference is small, and the first one is more commonly used than the second on. If both are simple, it may be better to document both stand-alone. The order depends on various factors. - If one is deprecated, start with the other one. - If one is used a lot more than the other, start with the common one. - Otherwise, if at least one of them has complicated effects, start with the simpler one. - If both are quite simple, it may be better to start with the more general one instead and describe the simpler one as special case afterwards. - If there are no stronger reasons, start with the one naming the manual page. In this case, you have to document event_base_new(3) before event_init(3) because event_base_new(3) is a non-trivial function and event_init(3) is merely a trivial wrapper around it. Actually, that justifies the order you picked for the NAME section, even though i tripped over that quirk earlier. > .Pp > The > .Lb libevent > API is generally not thread-safe unless only one > .Vt "event_base" > instance is accessible per thread or care is taken to lock access. That's way too early for saying such a thing. Threads are not an important concept and rarely used (if at all) in the OpenBSD base userland, so talking about them should be relegated to the end of the DESCRIPTION, or maybe even to CAVEATS. > .Pp > These functions allocate memory that should be freed using Avoid "should" in manual pages if possible. > .Xr event_base_free 3 . This means you have to document event_base_free(3) in the same manual page, not in a different one. In general, related alloc and free functions must not be separated into different pages. > .Pp > Diagnostic and error messages are retrievable from these functions by > configuring a message log callback with > .Xr event_set_log_callback 3 . Having this in the DESCRIPTION like this is awkward. Instead, maybe the ERRORS section should say that the event library usually does not set errno(2), but instead has its own system to issue messages via callbacks, pointing to the relevant manual page. (The exception that event_base_loop(3) does set EINTR needs to be specifically pointed out in the ERRORS section of event_base_loop(3) though.) If using event_set_log_callback(3) is almost always required when using libevent, maybe event_set_log_callback(3) should be in the same manual page as event_init(3), such that users aren't sent on a wild goose chase around lots of different pages. If, on the other hand, most programs get away without using event_set_log_callback(3), having it in a different page may be better because it might be less distracting. I never used libevent, so i'm not sure which is the case. I *suspect* event_set_log_callback(3) may usually be needed because maybe it is the only way to get any kind of diagnostics at all, which most programs probably need - but i'm not sure. > .Pp > After a call to > .Xr fork 2 > some notification methods will not persist and need to reinitiliazed with > .Xr event_reinit 3 . This seems misplaced. You did not yet explain what "notification methods" are. If those are related to event_set_log_callback(3), maybe this information belongs into that manual page? > .Sh RETURN VALUES Uh oh, end of the DESCRIPTION already? You failed to actually say what these two functions do. You should probably start somewhat like this: The function .Fn event_base_new allocates and initializes a new .Vt event_base structure .\ or "object", if you prefer, but remain consistent ... After that, it may or may not be useful to mention the most important fields (since struct event_base is opaque, do not talk about field names) and what they are initialized to, in particular those fields later discussed in the context of other API functions, though for many allocation APIs, it is not necessary to list all the fields, in particular not those that are purely internal implementation details. > The > .Fn event_base_new > and > .Fn event_init > functions return a > .Vt "struct event_base" > pointer. Not good because that is already obvious from the prototype. We somehow need a definite article here. Maybe something like: These functions return a pointer to the newly allocated .Vt event_base structure. Or even shorter, because it is not really ambiguous given the prototype: These functions return the newly allocated .Vt event_base structure. > If the functions fail, for example, due to a lack of memory, they do > .Em NOT > return. Never use all caps in the source code for emphasis. That serious harms accessibility because screenreaders will say "... they do En Oh Tee return." Also, the content is not good enough. It needs to say that the application program is terminated with the exit status 1. What a repugnant design choice, by the way... :-( Also, a DIAGNOSTICS section is clearly needed, listing the strings that get passed to event_err(3) and event_errx(3) from event_base_new(3) and explaining what they mean. > .Sh ENVIRONMENT > Programs using the > .Lb libevent > are configurable with environment variables prior to calling > .Fn event_base_new > or > .Fn event_init . That's excessively wordy, but lacks crucial information: what effects do the *values* of those varibles have? Maybe something like: Unless .Xr issetugid 2 is true, the following variables are inspected. The values of these variables are ignored, and they take effect even if their value is empty or zero. (I did not check all four variables.) > .Bl -tag mandoc: event_init.3:79:2: WARNING: missing -width in -tag list In case of doubt, say .Bl -tag -width Ds > .It Ev Sy EVENT_SHOW_METHOD The combination "Ev Sy" makes no sense. Both are in-line macros, and consequently, you cannot nest .Sy inside .Ev. Instead, as explained in the "In-line" subsection of mdoc(7), the .Sy closes the scope of the .Ev. Just say .It Ev EVENT_SHOW_METHOD here, the all caps syntax already stands out enough without boldface. > If the log callback is configured, report which kernel notification method the > library is using. This is not true for issetugid(2) programs. The same complaint may also apply to the other environment variables. > .It Ev Sy EVENT_NOKQUEUE > Disable library support for > .Xr kqueue 2 Missing full stop. > .It Ev Sy EVENT_NOPOLL > Disable library support for > .Xr poll 2 > .It Ev Sy EVENT_NOSELECT > Disable library support for > .Xr select 2 > .El > .Sh EXAMPLES > In this example code a new > .Vt "struct event_base" > is initialized Do not repeat the DESCRIPTION below EXAMPLES. > with support for logging. Maybe this example is more about logging than about new/free? In that case, it would belong into event_set_log_callback(3) rather than here. > This example also applies to > .Fn event_base_new , > the difference being that internal global variables are configured to simplify > other parts of the API. That information must go to the DESCRIPTION instead, where it is absolutely crucial, and you must be more specific about the global variables. Side effects need to be carefully documented, in particular on global variables, and even more so in functions where they are surprising, like in _new(3) functions. Again, what an utterly repugnant API design, but that's not your fault. > .Bd -literal -offset indent > #include <stdio.h> > #include <event.h> > > void cb(int sev, const char* msg) > { > printf("%d: %s\n", sev, msg); The backslash requires escaping: printf("%d: %s\en", sev, msg); > } > > int main() > { > event_set_log_callback(cb); > > struct event_base *base = event_init(); > if (base == NULL) { This does not make sense to me. My impression is that event_base_new(3) can never return NULL, and consequently, event_init(3) can't, either. So testing for it not only feels useless but misleading: it sets readers onto the wrong track and makes them misunderstand the API - doesn't it? > printf("event_init failed, will not return\\n"); You almost never want to use \\ in a manual page. Regarding the purpose of \\, see the documentation of the .de request in the roff(7) manual page. > return 1; > } > /* do something with the event library here */ > > event_base_free(base); > return 0; > } > .Ed > .Sh SEE ALSO > .Xr event_set_log_callback 3 , > .Xr event_base_free 3 , > .Xr event_dispatch 3 These need to be sorted alphabetically. > .Sh HISTORY > The function > .Fn event_init > first appeared in > .Lb libevent-0.1 No markup here. > although it's protoype has changed various times since that release. Too much information, just say something like and uses the current prototype since ... > In > .Lb libevent-1.4.1beta > .Fn event_base_new > was added and > .Fn event_init > was once again changed to wrap around the new function. Too much information, implementation details like this are not relevant for HISTORY. Something like The function .Fn event_base_new first appeared in libevent-1.4.1beta and has been available since .Ox ??? . should suffice. In general, we want to identify the OpenBSD release first supporting a function in addition to the place and time of original invention. Besides, is the "beta" really needed (for example because the beta appeared a very long time, like very many months or even several years before the official release and was widely used by applications before the official release)? Wouldn't it be better to just say "first appeared in libevent-1.4.1"? > .Pp > Environment variable options That sounds like a contradiction: options and environment variables are competing concepts and i don't think "environment variable option" is a thing. Maybe just something like Support for the environment variables first appeared in libevent-0.7a. > first appeared in > .Lb libevent-0.7a . No .Lb. > .Sh AUTHORS > The > .Lb libevent Just "The event library" seems enough. > was written by > .An -nosplit > .An Niels Provos > and > .An Nick Mathewson Missing full stop. > .Pp > This manual was written by > .An Ted Bullock Aq Mt tbull...@comlore.com Missing full stop. Do you want to draft a new version, taking this feedback into account? Yours, Ingo
