Re: [systemd-devel] [RFC] xyzctl-common
On Sat, 16.11.13 19:41, Peeters Simon ([email protected]) wrote: > hello all, > > During the sd_bus porting I noted that the *ctl tools (and > systemd-analyze) contain a lot of common boilerplate code. > > So the basic idea is to split this boilerplate out into > xyzctl-common.[ch] so that f.ex the bottom hostnamectl.c would look > like: There actually has been a lon-standing TODO item to unify the code that handles the "verbs" parsing in the various tools. In fact there's currently a bit too much copy/paste going on. For example, we have parsing code handling this in timedatectl, localectl, loginctl, systemctl at least. They are not identical bits of code though, but very similar (systemctl has some special magic to print bus errors only in some cases...). But either way: this should really be unified. > enum tool_flags { > XYZCTL_NO_REMOTE = 1, > Do not allow the use of --host=HOST to connect to a remote host > XYZCTL_NO_MACHINE = 2, > Do not allow the use of --machine=CONTAINER to connect to a container > XYZCTL_NO_USER = 4, > Do not allow connection to the user bus by using --user I really don't like "negative" bools in our code. It's bad enough we support them as command line switches, but internally we shouldn't create more like that... > XYZCTL_CONNECT_SYSTEMD = 8, > connect directly to systemd's private socket. > } Hmmm, I do like the verb_flags thing but I am not sold to the tool_flags idea... the systemd conenction thing for example appears so specific, that I wouldn't want to generalize that, especially since its going to go away with kdbus. So, I think for now, I'd just like to see unification of loginctl_main(), timedatectl_main(), localectl_main(), hostnamectl_main(), ... covering the verb flags, but I'd like to avoid the tool flags for now, let's leave this explicit in the tools, we can generalize this later on, should it really be necessary after kdbus is a requirement and we can drop the magic dbus code... Anyway, all work on this appreciated! (and srry for the long delay in reviewing, drowning in patches...) Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] xyzctl-common
2013/11/16 Zbigniew Jędrzejewski-Szmek :
> On Sat, Nov 16, 2013 at 07:41:13PM +0100, Peeters Simon wrote:
>> hello all,
>>
>> During the sd_bus porting I noted that the *ctl tools (and
>> systemd-analyze) contain a lot of common boilerplate code.
>>
>> So the basic idea is to split this boilerplate out into
>> xyzctl-common.[ch] so that f.ex the bottom hostnamectl.c would look
>> like:
>>
>> ...
>> static void help(void ) {...}
>>
>> enum {
>> ARG_TRANSIENT = 0x100,
>> ARG_STATIC,
>> ARG_PRETTY,
>> };
>>
>> static int parse_arg(int option, const char* arg) {
>> assert(option >= 0);
>>
>> switch (option) {
>> case ARG_TRANSIENT:
>> arg_transient = true;
>> break;
>>
>> case ARG_PRETTY:
>> arg_pretty = true;
>> break;
>>
>> case ARG_STATIC:
>> arg_static = true;
>> break;
>>
>> default:
>> return -EINVAL;
>> }
>>
>> return 1;
>> }
>>
>> int main(int argc, char *argv[]) {
>>
>> static const struct option options[] = {
>> { "transient", no_argument, NULL, ARG_TRANSIENT },
>> { "static",no_argument, NULL, ARG_STATIC},
>> { "pretty",no_argument, NULL, ARG_PRETTY},
>> {}
>> };
>> static const xyzctl_verb verbs[] = {
>> { "status",LESS, 1, show_status, XYZCTL_USE_PAGER
>> },
>> { "set-hostname", EQUAL, 2, set_hostname,
>> XYZCTL_USE_POLKIT },
>> { "set-icon-name", EQUAL, 2, set_icon_name,
>> XYZCTL_USE_POLKIT },
>> { "set-chassis", EQUAL, 2, set_chassis,
>> XYZCTL_USE_POLKIT },
>> {}
>> };
>> static const xyzctl_tool hostnamectl = {
>> &help,
>> &parse_arg,
>> XYZCTL_NO_USER,
>> options,
>> verbs
>> };
>>
>> return xyzctl_main(&hostnamectl, argc, argv);
>> }
>>
>> it would use or-able flags to controll common functionality, i currently
>> have:
>> enum verb_flags {
>> XYZCTL_NO_BUS = 1,
>> The execution of the verb does not require a bus connection.
>> The system will try to connect to the bus, but will ignore any errors,
>> and pass a NULL bus.
>> XYZCTL_USE_PAGER = 2,
>> Open a pager unless the user passes --no-pager
>> XYZCTL_USE_POLKIT = 4,
>> Starts the polkit agent unless the user passes --no-ask-password
>> XYZCTL_LOOP_ARGS = 8,
>> Executes the verb once for each argument.
>> This is usefull where
>> $ xyzctl verb a b c
>> is the same as
>> $ xyzctl verb a
>> $ xyzctl verb b
>> $ xyzctl verb c
> I think that we do some sorting on the arguments and other operations
> which mean that the behaviour for multiple args isn't usually
> *exactly* the same as for one repeated. What exactly does this
> flag mean?
what it does exactly:
instead of calling
verb.dispatch(bus, argv_left, argc_left)
it does:
for (int i = 1; i < argc_left; i++)
verb.dispatch(bus, (char *[]){argv_left[0], argv_left[i]}, 2);
an example use for it would be loginctl kill-session
In the beginning I tought this would be nice to have, but I am fine
with dropping this.
>> };
>>
>> enum tool_flags {
>> XYZCTL_NO_REMOTE = 1,
>> Do not allow the use of --host=HOST to connect to a remote host
>> XYZCTL_NO_MACHINE = 2,
>> Do not allow the use of --machine=CONTAINER to connect to a container
>> XYZCTL_NO_USER = 4,
>> Do not allow connection to the user bus by using --user
>> XYZCTL_CONNECT_SYSTEMD = 8,
>> connect directly to systemd's private socket.
>> }
>>
>>
>> As a proof of concept I ported hostnemctl and loginctl resulting in +-
>> 360 lines less code
>>
>>
>> Does anybody have any comments on the basic idea and/or the proposed
>> interface?
>
> If it makes things shorter, without making them much more complicated,
> then it's probably a good idea. Can you post the full patch for
> loginctl and hostnamectl?
I will do this tomorow.
on the calling side things get a lot simpler, the other side (the code
in xyzctl-common.c) is slightly more complex because it has to
dynamicly construct the options array for getopt_long, but I think it
is within reason.
___
systemd-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] xyzctl-common
On Sat, Nov 16, 2013 at 07:41:13PM +0100, Peeters Simon wrote:
> hello all,
>
> During the sd_bus porting I noted that the *ctl tools (and
> systemd-analyze) contain a lot of common boilerplate code.
>
> So the basic idea is to split this boilerplate out into
> xyzctl-common.[ch] so that f.ex the bottom hostnamectl.c would look
> like:
>
> ...
> static void help(void ) {...}
>
> enum {
> ARG_TRANSIENT = 0x100,
> ARG_STATIC,
> ARG_PRETTY,
> };
>
> static int parse_arg(int option, const char* arg) {
> assert(option >= 0);
>
> switch (option) {
> case ARG_TRANSIENT:
> arg_transient = true;
> break;
>
> case ARG_PRETTY:
> arg_pretty = true;
> break;
>
> case ARG_STATIC:
> arg_static = true;
> break;
>
> default:
> return -EINVAL;
> }
>
> return 1;
> }
>
> int main(int argc, char *argv[]) {
>
> static const struct option options[] = {
> { "transient", no_argument, NULL, ARG_TRANSIENT },
> { "static",no_argument, NULL, ARG_STATIC},
> { "pretty",no_argument, NULL, ARG_PRETTY},
> {}
> };
> static const xyzctl_verb verbs[] = {
> { "status",LESS, 1, show_status, XYZCTL_USE_PAGER
> },
> { "set-hostname", EQUAL, 2, set_hostname, XYZCTL_USE_POLKIT
> },
> { "set-icon-name", EQUAL, 2, set_icon_name, XYZCTL_USE_POLKIT
> },
> { "set-chassis", EQUAL, 2, set_chassis, XYZCTL_USE_POLKIT
> },
> {}
> };
> static const xyzctl_tool hostnamectl = {
> &help,
> &parse_arg,
> XYZCTL_NO_USER,
> options,
> verbs
> };
>
> return xyzctl_main(&hostnamectl, argc, argv);
> }
>
> it would use or-able flags to controll common functionality, i currently have:
> enum verb_flags {
> XYZCTL_NO_BUS = 1,
> The execution of the verb does not require a bus connection.
> The system will try to connect to the bus, but will ignore any errors,
> and pass a NULL bus.
> XYZCTL_USE_PAGER = 2,
> Open a pager unless the user passes --no-pager
> XYZCTL_USE_POLKIT = 4,
> Starts the polkit agent unless the user passes --no-ask-password
> XYZCTL_LOOP_ARGS = 8,
> Executes the verb once for each argument.
> This is usefull where
> $ xyzctl verb a b c
> is the same as
> $ xyzctl verb a
> $ xyzctl verb b
> $ xyzctl verb c
I think that we do some sorting on the arguments and other operations
which mean that the behaviour for multiple args isn't usually
*exactly* the same as for one repeated. What exactly does this
flag mean?
> };
>
> enum tool_flags {
> XYZCTL_NO_REMOTE = 1,
> Do not allow the use of --host=HOST to connect to a remote host
> XYZCTL_NO_MACHINE = 2,
> Do not allow the use of --machine=CONTAINER to connect to a container
> XYZCTL_NO_USER = 4,
> Do not allow connection to the user bus by using --user
> XYZCTL_CONNECT_SYSTEMD = 8,
> connect directly to systemd's private socket.
> }
>
>
> As a proof of concept I ported hostnemctl and loginctl resulting in +-
> 360 lines less code
>
>
> Does anybody have any comments on the basic idea and/or the proposed
> interface?
If it makes things shorter, without making them much more complicated,
then it's probably a good idea. Can you post the full patch for
loginctl and hostnamectl?
Zbyszek
___
systemd-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [RFC] xyzctl-common
hello all,
During the sd_bus porting I noted that the *ctl tools (and
systemd-analyze) contain a lot of common boilerplate code.
So the basic idea is to split this boilerplate out into
xyzctl-common.[ch] so that f.ex the bottom hostnamectl.c would look
like:
...
static void help(void ) {...}
enum {
ARG_TRANSIENT = 0x100,
ARG_STATIC,
ARG_PRETTY,
};
static int parse_arg(int option, const char* arg) {
assert(option >= 0);
switch (option) {
case ARG_TRANSIENT:
arg_transient = true;
break;
case ARG_PRETTY:
arg_pretty = true;
break;
case ARG_STATIC:
arg_static = true;
break;
default:
return -EINVAL;
}
return 1;
}
int main(int argc, char *argv[]) {
static const struct option options[] = {
{ "transient", no_argument, NULL, ARG_TRANSIENT },
{ "static",no_argument, NULL, ARG_STATIC},
{ "pretty",no_argument, NULL, ARG_PRETTY},
{}
};
static const xyzctl_verb verbs[] = {
{ "status",LESS, 1, show_status, XYZCTL_USE_PAGER },
{ "set-hostname", EQUAL, 2, set_hostname, XYZCTL_USE_POLKIT },
{ "set-icon-name", EQUAL, 2, set_icon_name, XYZCTL_USE_POLKIT },
{ "set-chassis", EQUAL, 2, set_chassis, XYZCTL_USE_POLKIT },
{}
};
static const xyzctl_tool hostnamectl = {
&help,
&parse_arg,
XYZCTL_NO_USER,
options,
verbs
};
return xyzctl_main(&hostnamectl, argc, argv);
}
it would use or-able flags to controll common functionality, i currently have:
enum verb_flags {
XYZCTL_NO_BUS = 1,
The execution of the verb does not require a bus connection.
The system will try to connect to the bus, but will ignore any errors,
and pass a NULL bus.
XYZCTL_USE_PAGER = 2,
Open a pager unless the user passes --no-pager
XYZCTL_USE_POLKIT = 4,
Starts the polkit agent unless the user passes --no-ask-password
XYZCTL_LOOP_ARGS = 8,
Executes the verb once for each argument.
This is usefull where
$ xyzctl verb a b c
is the same as
$ xyzctl verb a
$ xyzctl verb b
$ xyzctl verb c
};
enum tool_flags {
XYZCTL_NO_REMOTE = 1,
Do not allow the use of --host=HOST to connect to a remote host
XYZCTL_NO_MACHINE = 2,
Do not allow the use of --machine=CONTAINER to connect to a container
XYZCTL_NO_USER = 4,
Do not allow connection to the user bus by using --user
XYZCTL_CONNECT_SYSTEMD = 8,
connect directly to systemd's private socket.
}
As a proof of concept I ported hostnemctl and loginctl resulting in +-
360 lines less code
Does anybody have any comments on the basic idea and/or the proposed interface?
Simon
___
systemd-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
