Re: sndiod: Move controls out of the device structure, please test & review
On Fri, Jan 29, 2021 at 04:16:44PM +0100, Alexandre Ratchov wrote: > Thanks for the feedback, new diff below. It fixes a server crash when > a client issues an invalid request. I retested the crash scenario, and can confirm that it is fixed. No other problems to report! -- Best Regards Edd Barrett http://www.theunixzoo.co.uk
Re: sndiod: Move controls out of the device structure, please test & review
On Fri, Jan 29, 2021 at 12:42:37PM +0100, Alexandre Ratchov wrote:
> Moving to a global server-wide controls list is necessary to expose
> controls that are not associated to a particular device (ex. a device
> selector).
>
> The current hack to use the device-side sioctl_desc->addr variable as
> client-side key can't work anymore. So, we use a unique dynamically
> allocated ctl->addr key; this is which much cleaner. A new "scope"
> enum (with two "void *" arguments) is used to determine what the
> control does control. This adds a lot of flexibility and allows to
> easily add new control types that are not associated to devices.
>
> The diff touches many parts of the code and is quite big. Please test
> and report regressions.
>
Thanks for the feedback, new diff below. It fixes a server crash when
a client issues an invalid request.
Index: dev.c
===
RCS file: /cvs/src/usr.bin/sndiod/dev.c,v
retrieving revision 1.89
diff -u -p -u -p -r1.89 dev.c
--- dev.c 29 Jan 2021 11:38:23 - 1.89
+++ dev.c 29 Jan 2021 15:15:38 -
@@ -106,6 +106,7 @@ struct slotops zomb_slotops = {
zomb_exit
};
+struct ctl *ctl_list = NULL;
struct dev *dev_list = NULL;
unsigned int dev_sndnum = 0;
@@ -370,12 +371,14 @@ dev_midi_master(struct dev *d)
master = d->master;
else {
master = 0;
- for (c = d->ctl_list; c != NULL; c = c->next) {
+ for (c = ctl_list; c != NULL; c = c->next) {
if (c->type != CTL_NUM ||
- strcmp(c->group, "") != 0 ||
+ strcmp(c->group, d->name) != 0 ||
strcmp(c->node0.name, "output") != 0 ||
strcmp(c->func, "level") != 0)
continue;
+ if (c->u.any.arg0 != d)
+ continue;
v = (c->curval * 127 + c->maxval / 2) / c->maxval;
if (master < v)
master = v;
@@ -465,7 +468,7 @@ dev_midi_omsg(void *arg, unsigned char *
slot_array[chan].opt->dev != d)
return;
slot_setvol(slot_array + chan, msg[2]);
- dev_onval(d, CTLADDR_SLOT_LEVEL(chan), msg[2]);
+ ctl_onval(CTL_SLOT_LEVEL, slot_array + chan, NULL, msg[2]);
return;
}
x = (struct sysex *)msg;
@@ -479,7 +482,7 @@ dev_midi_omsg(void *arg, unsigned char *
if (len == SYSEX_SIZE(master)) {
dev_master(d, x->u.master.coarse);
if (d->master_enabled) {
- dev_onval(d, CTLADDR_MASTER,
+ ctl_onval(CTL_DEV_MASTER, d, NULL,
x->u.master.coarse);
}
}
@@ -1005,14 +1008,16 @@ dev_master(struct dev *d, unsigned int m
if (d->mode & MODE_PLAY)
dev_mix_adjvol(d);
} else {
- for (c = d->ctl_list; c != NULL; c = c->next) {
+ for (c = ctl_list; c != NULL; c = c->next) {
+ if (c->scope != CTL_HW || c->u.hw.dev != d)
+ continue;
if (c->type != CTL_NUM ||
- strcmp(c->group, "") != 0 ||
+ strcmp(c->group, d->name) != 0 ||
strcmp(c->node0.name, "output") != 0 ||
strcmp(c->func, "level") != 0)
continue;
v = (master * c->maxval + 64) / 127;
- dev_setctl(d, c->addr, v);
+ ctl_setval(c, v);
}
}
}
@@ -1070,7 +1075,7 @@ dev_new(char *path, struct aparams *par,
d->master = MIDI_MAXCTL;
d->mtc.origin = 0;
d->tstate = MMC_STOP;
- d->ctl_list = NULL;
+ snprintf(d->name, CTL_NAMEMAX, "%u", d->num);
d->next = dev_list;
dev_list = d;
return d;
@@ -1208,10 +1213,8 @@ dev_allocbufs(struct dev *d)
int
dev_open(struct dev *d)
{
- int i;
char name[CTL_NAMEMAX];
struct dev_alt *a;
- struct slot *s;
d->master_enabled = 0;
d->mode = d->reqmode;
@@ -1235,23 +1238,12 @@ dev_open(struct dev *d)
if (!dev_allocbufs(d))
return 0;
- for (i = 0, s = slot_array; i < DEV_NSLOT; i++, s++) {
- if (s->opt == NULL || s->opt->dev != d || s->name[0] == 0)
- continue;
- slot_ctlname(s, name, CTL_NAMEMAX);
- dev_addctl(d, "app", CTL_NUM,
- CTLADDR_SLOT_LEVEL(i),
- name, -1, "level",
-
sndiod: Move controls out of the device structure, please test & review
Moving to a global server-wide controls list is necessary to expose
controls that are not associated to a particular device (ex. a device
selector).
The current hack to use the device-side sioctl_desc->addr variable as
client-side key can't work anymore. So, we use a unique dynamically
allocated ctl->addr key; this is which much cleaner. A new "scope"
enum (with two "void *" arguments) is used to determine what the
control does control. This adds a lot of flexibility and allows to
easily add new control types that are not associated to devices.
The diff touches many parts of the code and is quite big. Please test
and report regressions.
Besides that no behavior change.
OK?
Index: dev.c
===
RCS file: /cvs/src/usr.bin/sndiod/dev.c,v
retrieving revision 1.89
diff -u -p -u -p -r1.89 dev.c
--- dev.c 29 Jan 2021 11:38:23 - 1.89
+++ dev.c 29 Jan 2021 11:39:08 -
@@ -106,6 +106,7 @@ struct slotops zomb_slotops = {
zomb_exit
};
+struct ctl *ctl_list = NULL;
struct dev *dev_list = NULL;
unsigned int dev_sndnum = 0;
@@ -370,12 +371,14 @@ dev_midi_master(struct dev *d)
master = d->master;
else {
master = 0;
- for (c = d->ctl_list; c != NULL; c = c->next) {
+ for (c = ctl_list; c != NULL; c = c->next) {
if (c->type != CTL_NUM ||
- strcmp(c->group, "") != 0 ||
+ strcmp(c->group, d->name) != 0 ||
strcmp(c->node0.name, "output") != 0 ||
strcmp(c->func, "level") != 0)
continue;
+ if (c->u.any.arg0 != d)
+ continue;
v = (c->curval * 127 + c->maxval / 2) / c->maxval;
if (master < v)
master = v;
@@ -465,7 +468,7 @@ dev_midi_omsg(void *arg, unsigned char *
slot_array[chan].opt->dev != d)
return;
slot_setvol(slot_array + chan, msg[2]);
- dev_onval(d, CTLADDR_SLOT_LEVEL(chan), msg[2]);
+ ctl_onval(CTL_SLOT_LEVEL, slot_array + chan, NULL, msg[2]);
return;
}
x = (struct sysex *)msg;
@@ -479,7 +482,7 @@ dev_midi_omsg(void *arg, unsigned char *
if (len == SYSEX_SIZE(master)) {
dev_master(d, x->u.master.coarse);
if (d->master_enabled) {
- dev_onval(d, CTLADDR_MASTER,
+ ctl_onval(CTL_DEV_MASTER, d, NULL,
x->u.master.coarse);
}
}
@@ -1005,14 +1008,16 @@ dev_master(struct dev *d, unsigned int m
if (d->mode & MODE_PLAY)
dev_mix_adjvol(d);
} else {
- for (c = d->ctl_list; c != NULL; c = c->next) {
+ for (c = ctl_list; c != NULL; c = c->next) {
+ if (c->scope != CTL_HW || c->u.hw.dev != d)
+ continue;
if (c->type != CTL_NUM ||
- strcmp(c->group, "") != 0 ||
+ strcmp(c->group, d->name) != 0 ||
strcmp(c->node0.name, "output") != 0 ||
strcmp(c->func, "level") != 0)
continue;
v = (master * c->maxval + 64) / 127;
- dev_setctl(d, c->addr, v);
+ ctl_setval(c, v);
}
}
}
@@ -1070,7 +1075,7 @@ dev_new(char *path, struct aparams *par,
d->master = MIDI_MAXCTL;
d->mtc.origin = 0;
d->tstate = MMC_STOP;
- d->ctl_list = NULL;
+ snprintf(d->name, CTL_NAMEMAX, "%u", d->num);
d->next = dev_list;
dev_list = d;
return d;
@@ -1208,10 +1213,8 @@ dev_allocbufs(struct dev *d)
int
dev_open(struct dev *d)
{
- int i;
char name[CTL_NAMEMAX];
struct dev_alt *a;
- struct slot *s;
d->master_enabled = 0;
d->mode = d->reqmode;
@@ -1235,23 +1238,12 @@ dev_open(struct dev *d)
if (!dev_allocbufs(d))
return 0;
- for (i = 0, s = slot_array; i < DEV_NSLOT; i++, s++) {
- if (s->opt == NULL || s->opt->dev != d || s->name[0] == 0)
- continue;
- slot_ctlname(s, name, CTL_NAMEMAX);
- dev_addctl(d, "app", CTL_NUM,
- CTLADDR_SLOT_LEVEL(i),
- name, -1, "level",
- NULL, -1, 127, s->vol);
- }
-
/* if there are multiple alt devs, add server.device knob */
if (d->alt_list->next != NULL) {
