Re: Avoid system(3) in ikectl
ping
On Fri, Mar 8, 2019 at 8:52 PM Matthew Martin wrote:
>
> On Fri, Mar 8, 2019 at 3:39 AM Reyk Floeter wrote:
> >
> > On Wed, Mar 06, 2019 at 10:42:15PM -0600, Matthew Martin wrote:
> > > I had sent a similar patch a while back. There seemed to me some
> > > interest, but it was never comitted. Updated to apply to -current.
> > >
> >
> > I vaguely remember that there was a diff that had issues that I didn't
> > like for different reasons. The explanation in your other mail (about
> > using it with doas, escaping of special characters) make some sense,
> > so I wouldn't be opposed to the idea in general.
> >
> > But the diff is not OK as it is; a few suggestions:
> >
> > - I see that "PATH_MAX + 5" is for the "file:" prefix. This lacks a
> > comment as it looks confusing at first sight.
> >
> > - I don't like the way how you run your run() function: declaring a
> > *cmd[] variable just before calling the function, very often in a
> > nested {} block, doesn't align to the style C code is written in
> > OpenBSD.
> >
> > I think I have also commented the last time that you should use an
> > execl-interface instead that uses varags terminated by a NULL instead.
> >
> > int
> > ca_system(const char *arg, ...);
> >
> > for example
> > ca_system(PATH_OPENSSL, "x509", "-h", NULL);
> >
> > You can re-construct the argv[] in the function internally, looping
> > over the va_list.
> >
> > - And why does run() return int if you never check the return value?
> > The current system() calls are not checked either but that doesn't
> > mean that a local function would have to define an unused return
> > value. Blindly err'ing out in the function wouldn't help either as I
> > guess that there are a few calls that might fail if something exists
> > but allow ikectl to proceed without problems.
> >
> > Reyk
>
> I believe I've addressed your comments. The code for turning the varargs
> into argv is from execl in lib/libc/gen/exec.c. If the allocation for
> argv fails, instead of returning -1 with ENOMEM, I thought it makes more
> sense to err out.
diff --git usr.sbin/ikectl/ikeca.c usr.sbin/ikectl/ikeca.c
index bac76ab9c2f..09df5066820 100644
--- usr.sbin/ikectl/ikeca.c
+++ usr.sbin/ikectl/ikeca.c
@@ -18,11 +18,13 @@
#include
#include
+#include
#include
#include
#include
#include
#include
+#include
#include
#include
#include
@@ -63,12 +65,12 @@
struct ca {
char sslpath[PATH_MAX];
- char passfile[PATH_MAX];
+ char passfile[PATH_MAX + 5]; // Includes the "file:" prefix
char index[PATH_MAX];
char serial[PATH_MAX];
char sslcnf[PATH_MAX];
char extcnf[PATH_MAX];
- char batch[PATH_MAX];
+ char *batch;
char *caname;
};
@@ -116,6 +118,7 @@ void ca_setenv(const char *, const char *);
void ca_clrenv(void);
void ca_setcnf(struct ca *, const char *);
void ca_create_index(struct ca *);
+static void ca_system(char *, ...);
/* util.c */
int expand_string(char *, size_t, const char *, const char *);
@@ -130,7 +133,6 @@ int
ca_key_create(struct ca *ca, char *keyname)
{
struct stat st;
- char cmd[PATH_MAX * 2];
char path[PATH_MAX];
snprintf(path, sizeof(path), "%s/private/%s.key", ca->sslpath, keyname);
@@ -140,10 +142,7 @@ ca_key_create(struct ca *ca, char *keyname)
return (0);
}
- snprintf(cmd, sizeof(cmd),
- "%s genrsa -out %s 2048",
- PATH_OPENSSL, path);
- system(cmd);
+ ca_system(PATH_OPENSSL, "genrsa", "-out", path, "2048", NULL);
chmod(path, 0600);
return (0);
@@ -200,9 +199,9 @@ ca_delkey(struct ca *ca, char *keyname)
int
ca_request(struct ca *ca, char *keyname, int type)
{
- char cmd[PATH_MAX * 2];
char hostname[HOST_NAME_MAX+1];
char name[128];
+ char key[PATH_MAX];
char path[PATH_MAX];
ca_setenv("$ENV::CERT_CN", keyname);
@@ -226,13 +225,11 @@ ca_request(struct ca *ca, char *keyname, int type)
ca_setcnf(ca, keyname);
+ snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname);
snprintf(path, sizeof(path), "%s/private/%s.csr", ca->sslpath, keyname);
- snprintf(cmd, sizeof(cmd), "%s req %s-new"
- " -key %s/private/%s.key -out %s -config %s",
- PATH_OPENSSL, ca->batch, ca->sslpath, keyname,
- path, ca->sslcnf);
- system(cmd);
+ ca_system(PATH_OPENSSL, "req", "-new", "-key", key, "-out", path,
+ "-config", ca->sslcnf, ca->batch, NULL);
chmod(path, 0600);
return (0);
@@ -241,8 +238,11 @@ ca_request(struct ca *ca, char *keyname, int type)
int
ca_sign(struct ca *ca, char *keyname, int type)
{
- char cmd[PATH_MAX * 2];
- const char *extensions = NULL;
+ char cakey[PATH_MAX];
+ char cacrt[PATH_MAX];
+ char out[PATH_MAX];
+ char in[PATH_MAX];
+ char *extensions = NULL;
if (type == HOST_IPADDR) {
extensions = "x509v3_IPAddr";
@@ -258,19 +258,15 @@ ca_sign(struct ca *ca, char *keyname, int type)
ca_setenv("$ENV::CASERIAL", ca->serial);
ca_setcnf(ca, keyname);
- snprintf(cmd, sizeof(cmd),
- "%s ca -config %s -keyfile
Re: vmd/vmctl: improve VM name checks/error handling
On Sat, Mar 16, 2019 at 11:41:02PM +, Jason McIntyre wrote: > i don;t understand why you special case "id" in a separate paragraph. Specifying an ID is valid only if you want to start an existing VM. You cannot create new VMs using a numerical name, that is an ID. This limitation is the point I wanted to clarify in the first place. Without the separate paragraph, this information is lost and `id | name' could just be `id' again. But as shown in one of my previous mails, something like `vmctl start 60 ...' to create a new VM called "60" is not possible and leads to non-obvious errors, hence my attempt to differentiate between those cases. > just document "name" and "id" as normal arguments. That's what I just did, no? > within the "start" command it would be consistent, but within the page > less so. i think this should be fixed wholesale, in a separate diff. Sure.
Re: vmd/vmctl: improve VM name checks/error handling
On Sun, Mar 17, 2019 at 12:21:32AM +0100, Klemens Nanni wrote:
> On Sat, Mar 16, 2019 at 10:22:14PM +, Jason McIntyre wrote:
> > i think more properly we should show
> >
> > -t id | name
> It's about referencing the VM to be started itself, not templates.
> `-t id' is not possible.
>
ah, i thought this was -t we were discussing.
> But you pointed out how my addition would errornously imply that, so
> change the `start' synopsis from `name' to `id | name' as well as the
> command description to differentiate between both cases.
>
i don;t understand why you special case "id" in a separate paragraph.
just document "name" and "id" as normal arguments.
> "Starts" to "Start" while here.
>
within the "start" command it would be consistent, but within the page
less so. i think this should be fixed wholesale, in a separate diff.
> Is that clearer?
>
> Index: usr.sbin/vmctl/vmctl.8
> ===
> RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v
> retrieving revision 1.61
> diff -u -p -r1.61 vmctl.8
> --- usr.sbin/vmctl/vmctl.87 Mar 2019 18:54:05 - 1.61
> +++ usr.sbin/vmctl/vmctl.816 Mar 2019 23:06:31 -
> @@ -144,7 +144,7 @@ under the same path.
> An alias for the
> .Cm status
> command.
> -.It Xo Cm start Ar name
> +.It Xo Cm start Ar id | name
> .Op Fl cL
> .Bk -words
> .Op Fl B Ar device
> @@ -157,7 +157,11 @@ command.
> .Op Fl t Ar name
> .Ek
> .Xc
> -Starts a VM defined by the specified name and parameters:
> +Start a new VM
> +.Ar name
> +with the specified parameters.
> +An existing VM may be referenced by its
> +.Ar id .
> .Bl -tag -width "-I parent"
> .It Fl B Ar device
> Force system to boot from the specified device for this boot.
> Index: usr.sbin/vmctl/main.c
> ===
> RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 main.c
> --- usr.sbin/vmctl/main.c 1 Mar 2019 12:47:36 - 1.54
> +++ usr.sbin/vmctl/main.c 16 Mar 2019 22:42:24 -
> @@ -81,7 +81,7 @@ struct ctl_command ctl_commands[] = {
> { "reset", CMD_RESET, ctl_reset, "[all | switches |
> vms]" },
> { "send", CMD_SEND, ctl_send, "id", 1},
> { "show", CMD_STATUS, ctl_status, "[id]" },
> - { "start", CMD_START, ctl_start, "name"
> + { "start", CMD_START, ctl_start, "id | name"
> " [-cL] [-B device] [-b path] [-d disk] [-i count]\n"
> "\t\t[-m size] [-n switch] [-r path] [-t name]" },
> { "status", CMD_STATUS, ctl_status, "[id]" },
>
Re: vmd/vmctl: improve VM name checks/error handling
On Sat, Mar 16, 2019 at 10:22:14PM +, Jason McIntyre wrote:
> i think more properly we should show
>
> -t id | name
It's about referencing the VM to be started itself, not templates.
`-t id' is not possible.
But you pointed out how my addition would errornously imply that, so
change the `start' synopsis from `name' to `id | name' as well as the
command description to differentiate between both cases.
"Starts" to "Start" while here.
Is that clearer?
Index: usr.sbin/vmctl/vmctl.8
===
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v
retrieving revision 1.61
diff -u -p -r1.61 vmctl.8
--- usr.sbin/vmctl/vmctl.8 7 Mar 2019 18:54:05 - 1.61
+++ usr.sbin/vmctl/vmctl.8 16 Mar 2019 23:06:31 -
@@ -144,7 +144,7 @@ under the same path.
An alias for the
.Cm status
command.
-.It Xo Cm start Ar name
+.It Xo Cm start Ar id | name
.Op Fl cL
.Bk -words
.Op Fl B Ar device
@@ -157,7 +157,11 @@ command.
.Op Fl t Ar name
.Ek
.Xc
-Starts a VM defined by the specified name and parameters:
+Start a new VM
+.Ar name
+with the specified parameters.
+An existing VM may be referenced by its
+.Ar id .
.Bl -tag -width "-I parent"
.It Fl B Ar device
Force system to boot from the specified device for this boot.
Index: usr.sbin/vmctl/main.c
===
RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
retrieving revision 1.54
diff -u -p -r1.54 main.c
--- usr.sbin/vmctl/main.c 1 Mar 2019 12:47:36 - 1.54
+++ usr.sbin/vmctl/main.c 16 Mar 2019 22:42:24 -
@@ -81,7 +81,7 @@ struct ctl_command ctl_commands[] = {
{ "reset", CMD_RESET, ctl_reset, "[all | switches |
vms]" },
{ "send", CMD_SEND, ctl_send, "id", 1},
{ "show", CMD_STATUS, ctl_status, "[id]" },
- { "start", CMD_START, ctl_start, "name"
+ { "start", CMD_START, ctl_start, "id | name"
" [-cL] [-B device] [-b path] [-d disk] [-i count]\n"
"\t\t[-m size] [-n switch] [-r path] [-t name]" },
{ "status", CMD_STATUS, ctl_status, "[id]" },
Re: vmd/vmctl: improve VM name checks/error handling
On Sat, Mar 16, 2019 at 09:31:39PM +0100, Klemens Nanni wrote: > On Thu, Mar 07, 2019 at 10:21:50PM +0100, Klemens Nanni wrote: > > # vmctl start 1 > > vmctl: started vm 1 successfully, tty /dev/ttypo > > # vmctl stop 1 -f > > stopping vm: forced to terminate vm 1 > > # vmctl start a > > vmctl: started vm 2 successfully, tty /dev/ttypo > > > > That is wonky, but I see how `start' must indeed accept IDs in the case > > of predefined VMs. > > > > I'll work on docs to clarify this further without touching code, thanks. > How about this? > > Index: usr.sbin/vmctl/vmctl.8 > === > RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v > retrieving revision 1.61 > diff -u -p -r1.61 vmctl.8 > --- usr.sbin/vmctl/vmctl.87 Mar 2019 18:54:05 - 1.61 > +++ usr.sbin/vmctl/vmctl.816 Mar 2019 20:31:07 - > @@ -233,6 +233,11 @@ The instance will inherit settings from > except for exclusive options such as disk, interface lladdr, or > interface names. > .El > +.Pp > +For existing VMs, > +.Ar name > +may be the numerical > +.Ar id . > .It Cm status Op Ar id > Lists VMs running on the host, optionally listing just the selected VM > .Ar id . > hi. i think more properly we should show -t id | name then all the bits that refer to "id" in that doc make sense. and your text can just say Use an existing VM with the specified .Ar id or .Ar name as a template ... jmc
Re: vmd/vmctl: improve VM name checks/error handling
On Thu, Mar 07, 2019 at 10:21:50PM +0100, Klemens Nanni wrote: > # vmctl start 1 > vmctl: started vm 1 successfully, tty /dev/ttypo > # vmctl stop 1 -f > stopping vm: forced to terminate vm 1 > # vmctl start a > vmctl: started vm 2 successfully, tty /dev/ttypo > > That is wonky, but I see how `start' must indeed accept IDs in the case > of predefined VMs. > > I'll work on docs to clarify this further without touching code, thanks. How about this? Index: usr.sbin/vmctl/vmctl.8 === RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v retrieving revision 1.61 diff -u -p -r1.61 vmctl.8 --- usr.sbin/vmctl/vmctl.8 7 Mar 2019 18:54:05 - 1.61 +++ usr.sbin/vmctl/vmctl.8 16 Mar 2019 20:31:07 - @@ -233,6 +233,11 @@ The instance will inherit settings from except for exclusive options such as disk, interface lladdr, or interface names. .El +.Pp +For existing VMs, +.Ar name +may be the numerical +.Ar id . .It Cm status Op Ar id Lists VMs running on the host, optionally listing just the selected VM .Ar id .
rasops(9): remove filtering in rasops_list_font_cb()
Hi tech@,
Here is a diff to modify rasops_list_font_cb() to not filter out fonts
with different sizes than the currently used one. This allows getting a
list of all loaded fonts when using the WSDISPLAYIO_LSFONT ioctl.
Currently, the output of wsfontload -l on my machine is:
# Name Encoding W H
0 Spleen 12x24 iso 12 24
With the diff applied:
# Name Encoding W H
0 Spleen 8x16 iso 8 16
1 Spleen 12x24 iso 12 24
2 Spleen 16x32 iso 16 32
3 Spleen 32x64 iso 32 64
On top of allowing to see all fonts which are currently loaded, it
removes the confusion as to why we reach the WSDISPLAY_MAXFONTCOUNT
limit faster than expected when loading more fonts.
It is also a first step towards allowing loading and enabling fonts
of different sizes.
Comments? OK?
Index: sys/dev/rasops/rasops.c
===
RCS file: /cvs/src/sys/dev/rasops/rasops.c,v
retrieving revision 1.58
diff -u -p -r1.58 rasops.c
--- sys/dev/rasops/rasops.c 9 Jan 2019 11:23:32 - 1.58
+++ sys/dev/rasops/rasops.c 15 Mar 2019 22:57:43 -
@@ -1899,10 +1899,6 @@ rasops_list_font_cb(void *cbarg, struct
{
struct rasops_list_font_ctx *ctx = cbarg;
- if (font->fontheight != ctx->ri->ri_font->fontheight ||
- font->fontwidth != ctx->ri->ri_font->fontwidth)
- return 0;
-
if (ctx->cnt-- == 0) {
ctx->font = font;
return 1;
ihidev: always register interrupt
I still haven't figured out why ihidev doesn't receive interrupts
for I2C devices on Intel 100 series and newer.
But on an Intel 300 series laptop, I noticed that the Elan
touchscreen (ims at ihidev) does actually generate interrupts while
the Elan touchpad (imt at ihidev) doesn't. In this scenario, we can
disable polling on the touchscreen to save some power and make input
smoother.
This diff always sets up the interrupt even when polling is
requested, and if the interrupt handler ever fires when polling is
enabled, disable polling.
Index: dev/acpi/dwiic_acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/dwiic_acpi.c,v
retrieving revision 1.8
diff -u -p -u -p -r1.8 dwiic_acpi.c
--- dev/acpi/dwiic_acpi.c 1 Jul 2018 11:37:11 - 1.8
+++ dev/acpi/dwiic_acpi.c 16 Mar 2019 15:39:34 -
@@ -462,8 +462,9 @@ dwiic_acpi_found_ihidev(struct dwiic_sof
aml_freevalue();
- if (!sc->sc_poll_ihidev &&
- !(crs.irq_int == 0 && crs.gpio_int_node == NULL))
+ if (sc->sc_poll_ihidev)
+ ia.ia_poll = 1;
+ if (!(crs.irq_int == 0 && crs.gpio_int_node == NULL))
ia.ia_intr =
if (config_found(sc->sc_iic, , dwiic_i2c_print)) {
Index: dev/i2c/i2cvar.h
===
RCS file: /cvs/src/sys/dev/i2c/i2cvar.h,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 i2cvar.h
--- dev/i2c/i2cvar.h23 Apr 2016 09:40:28 - 1.16
+++ dev/i2c/i2cvar.h16 Mar 2019 15:39:34 -
@@ -113,6 +113,7 @@ struct i2c_attach_args {
char*ia_name; /* chip name */
void*ia_cookie; /* pass extra info from bus to dev */
void*ia_intr; /* interrupt info */
+ int ia_poll;/* to force polling */
};
/*
Index: dev/i2c/ihidev.c
===
RCS file: /cvs/src/sys/dev/i2c/ihidev.c,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 ihidev.c
--- dev/i2c/ihidev.c20 Sep 2018 01:19:56 - 1.18
+++ dev/i2c/ihidev.c16 Mar 2019 15:39:34 -
@@ -128,7 +128,7 @@ ihidev_attach(struct device *parent, str
printf(", can't establish interrupt");
}
- if (sc->sc_ih == NULL) {
+ if (ia->ia_poll) {
printf(" (polling)");
sc->sc_poll = 1;
sc->sc_fastpoll = 1;
@@ -580,6 +580,16 @@ ihidev_hid_desc_parse(struct ihidev_soft
return (0);
}
+void
+ihidev_poll(void *arg)
+{
+ struct ihidev_softc *sc = arg;
+
+ sc->sc_dopoll = 1;
+ ihidev_intr(sc);
+ sc->sc_dopoll = 0;
+}
+
int
ihidev_intr(void *arg)
{
@@ -590,6 +600,13 @@ ihidev_intr(void *arg)
u_char *p;
u_int rep = 0;
+ if (sc->sc_poll && !sc->sc_dopoll) {
+ printf("%s: received interrupt while polling, disabling "
+ "polling\n", sc->sc_dev.dv_xname);
+ sc->sc_poll = 0;
+ timeout_del(>sc_timer);
+ }
+
/*
* XXX: force I2C_F_POLL for now to avoid dwiic interrupting
* while we are interrupting
@@ -741,7 +758,7 @@ ihidev_open(struct ihidev *scd)
if (sc->sc_poll) {
if (!timeout_initialized(>sc_timer))
- timeout_set(>sc_timer, (void *)ihidev_intr, sc);
+ timeout_set(>sc_timer, (void *)ihidev_poll, sc);
if (!timeout_pending(>sc_timer))
timeout_add(>sc_timer, FAST_POLL_MS);
}
Index: dev/i2c/ihidev.h
===
RCS file: /cvs/src/sys/dev/i2c/ihidev.h,v
retrieving revision 1.6
diff -u -p -u -p -r1.6 ihidev.h
--- dev/i2c/ihidev.h25 Aug 2018 18:32:05 - 1.6
+++ dev/i2c/ihidev.h16 Mar 2019 15:39:34 -
@@ -89,6 +89,7 @@ struct ihidev_softc {
int sc_refcnt;
int sc_poll;
+ int sc_dopoll;
int sc_fastpoll;
struct timeout sc_timer;
};
Re: wscons: precision scrolling
On Thu, 14 Mar 2019 at 00:48:33 +0100, Ulf Brosziewski wrote: > Much too fast? I'm a bit surprised. In my tests, the new method was > generally somewhat slower than the old one (and I haven't changed the > "scroll units"). How did you test it? Which hardware and which applications > did you use? This is with an imt touchpad: ihidev0 at iic1 addr 0x15 irq 109 (polling), vendor 0x4f3 product 0x3056, ELAN2201 ihidev0: 93 report ids imt0 at ihidev0: clickpad, 5 contacts wsmouse0 at imt0 mux 0 I tested with two-finger scrolling in Firefox and Chrome with a new user and fresh browser profiles. I loaded the same webpages with the old wstpad code and the new, and the two-finger scrolling with the new setup scrolls much too fast for even small finger movements (~0.5"). A movement with the previous code that would trigger the mousewheel-style scrolling of just a handful of lines causes the new code to scroll nearly an entire screen-height of the page.
Re: Option for alternative escape character with cu(1)
On Fri, Mar 15, 2019 at 09:51:12PM +, Nicholas Marriott wrote:
> On Fri, Mar 15, 2019 at 09:43:56AM -0600, Todd C. Miller wrote:
> > Wouldn't it be less error-prone to make escape_char u_char instead
> > of int?
>
> Maybe, I don't mind either way.
>
> However this in stream_read would still need a cast as ptr is signed:
>
> if (state_change && (u_char)*ptr == escape_char) {
>
> Although there is no reason ptr couldn't be u_char * too (in that case
> it would be nice to change line_read as well).
do_command(char c) was why I didn't go for u_char, and kept it int as
it was in ssh.
I've done some builds of cu with -Wpointer-sign etc., but the output
didn't seem anything like i would like to minimally solve, or cause to
anyone else. :]
-Artturi
