[systemd-devel] Bug#727708: [PATCH] systemctl: allow globbing in commands which take multiple unit names

[Lennart Poettering]

On Thu, 26.12.13 10:09, Russ Allbery (rra at debian.org) wrote:

> 
> Zbigniew Jędrzejewski-Szmek <zbyszek at in.waw.pl> writes:
> 
> > As discussed on IRC, here's a patch which takes the simple approach of
> > allowing globbing on loaded unit names in various (almost all :)) systemctl
> > commands. Comments?
> 
> I don't know the coding style of systemd, so please feel free to ignore
> this comment if it's not consistent with the style used elsewhere.  But
> since I saw this in passing:
> 
> > diff --git a/src/core/device.c b/src/core/device.c
> > index 2c44dec..d3976c9 100644
> > --- a/src/core/device.c
> > +++ b/src/core/device.c
> > @@ -268,7 +268,7 @@ static int device_update_unit(Manager *m, struct udev_device *dev, const char *p
> >                                  memcpy(e, w, l);
> >                                  e[l] = 0;
> >  
> > -                                n = unit_name_mangle(e);
> > +                                n = unit_name_mangle(e, false);
> >                                  if (!n) {
> >                                          r = -ENOMEM;
> >                                          goto fail;
> 
> A wise person convinced me a while back to avoid boolean arguments to C
> functions in most cases because the meaning of the argument is very
> inobvious at the call site.  "false" what?  What's disabled?  One has to
> go read the function definition to know, and while that's easy to find,
> particularly with a cross-referencing editor, it takes one more step.
> 
> What I've switched to instead is using tiny enums for this purpose.  So:
> 
>     enum mangle_type {
>         MANGLE_NOGLOB = 0,
>         MANGLE_GLOB   = 1
>     };
> 
> and then at the call site:
> 
>     n = unit_name_mangle(e, MANGLE_NOGLOB);
> 
> which makes the meaning of that argument immediately obvious.

As long as this is just one boolean arg on functions, or the functions
are internally used I think booleans are fine to use.

I don't think flag fields are necessarily the best choice for APIs in
general. For APIs that are built around a context object seperate
boolean setter calls are the better choice (i.e. foobar_set_waldo() to
set som boolean called "waldo" on a context object "foobar). 

Then in many APIs we exposed we actually return strings instead of
enums, since that's much easier to extend. (libudev does this for
example, and so does libsystemd-login)

So, the summary is: keep it low on booleans, keep it low on flags. The
middle ground where you don't have much of neither is the best, and in
general having fewer options is better than having many...

Lennart

-- 
Lennart Poettering, Red Hat


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST at lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster at lists.debian.org