Bug 231441
| Summary: | 12.0-ALPHA6 network does not start at boot | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Patrick McMunn <doctorwhoguy> | ||||
| Component: | misc | Assignee: | Warner Losh <imp> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Many People | CC: | amvandemore, bz, johalun0, net, re, rgrimes, yuripv | ||||
| Priority: | --- | Keywords: | regression | ||||
| Version: | CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| URL: | https://reviews.freebsd.org/D17267 | ||||||
| Attachments: |
|
||||||
|
Description
Patrick McMunn
2018-09-18 03:45:05 UTC
Here's some devd output: https://gist.github.com/johalun/c9383ff03cb29e12c3f4978e06ca6413#file-gistfile1-txt-L761-L762 The highlighted lines show suspicious regexps. By the way, this affect many people so it's very important. (In reply to Patrick from comment #0) Can you please attach your rc.conf to this ticket? there's a chance this is related to a devd bug someone contacted me about.... it looked similar to Johannes' output... I'll have to investigate. Someone on the current mailing list was also saying that this is a devd issue. Here's my current rc.conf: clear_tmp_enable="YES" syslogd_flags="-ss" sendmail_enable="NONE" hostname="xps400" dbus_enable="YES" hald_enable="YES" moused_enable="YES" ntpdate_enable="YES" ntpd_enable="YES" powerd_enable="YES" sddm_enable="YES" sshd_enable="YES" webcamd_enable="YES" # Set dumpdev to "AUTO" to enable crash dumps, "NO" to disable dumpdev="AUTO" wlans_run0="wlan0" ifconfig_wlan0="DHCP WPA" Try this instead: ifconfig_wlan0="WPA SYNCDHCP" (In reply to amvandemore from comment #6) I made the change you recommended, but it made no difference. I still had to manually run "service netif start" to bring up the network after boot. FWIW, I'm not seeing any issues with if_run loaded from /boot/loader.conf, it's correctly brought up on boot with the following (standard) entries in rc.conf: wlans_run0="wlan0" ifconfig_wlan0="WPA SYNCDHCP" (In reply to Yuri Pankov from comment #8) When I boot my system, kldstat shows that if_run.ko is loaded, and dmesg shows: run0 on uhub2 uhid0 on uhub4 uhid0: <Logitech Logitech Dual Action, class 0/0, rev 1.10/3.00, addr 2> on usbus1 run0: <1.0> on usbus4 run0: MAC/BBP RT3070 (rev 0x0201), RF RT3020 (MIMO 1T1R), address 00:12:0e:ce:c5:04 After manually starting the network, dmesg shows: wlan0: Ethernet address: 00:12:0e:ce:c5:04 run0: firmware RT2870 ver. 0.33 loaded wlan0: link state changed to UP So the driver, but not the firmware, is being loaded automatically at boot. I don't know if that's significant or if the firmware normally isn't loaded until the network is brought up. Apparently, I'm being stupid, and the use case here is NOT to load driver via loader.conf and rather rely on devd to load it and bring up wlan interface. To make up for my mistake, I've taken a look at the source of problem here.
Looks like devd::expand_one() treats "$(var)" as a subcommand to be passed down to shell -- this works correctly for the items in "action", but not so good for device matching. In any case, removing parentheses from the wifi-driver-regex seems to work for me loading the if_run.ko, and creating wlan0:
diff --git a/sbin/devd/devd.conf b/sbin/devd/devd.conf
index 956a2970d436..c9deaf5dada9 100644
--- a/sbin/devd/devd.conf
+++ b/sbin/devd/devd.conf
@@ -19,13 +19,13 @@ options {
# Setup some shorthand for regex that we use later in the file.
#XXX Yes, these are gross -- imp
set scsi-controller-regex
- "(aac|adv|adw|aha|ahc|ahd|aic|amr|bt|ciss|dpt|\
+ "aac|adv|adw|aha|ahc|ahd|aic|amr|bt|ciss|dpt|\
esp|ida|iir|ips|isp|mlx|mly|mpr|mps|mpt|ncr|ncv|nsp|stg|sym|\
- trm)\
+ trm\
[0-9]+";
set wifi-driver-regex
- "(ath|bwi|bwn|ipw|iwi|iwm|iwn|malo|mwl|ral|rsu|rtwn|rum|run|\
- uath|upgt|ural|urtw|wi|wpi|wtap|zyd)[0-9]+";
+ "ath|bwi|bwn|ipw|iwi|iwm|iwn|malo|mwl|ral|rsu|rtwn|rum|run|\
+ uath|upgt|ural|urtw|wi|wpi|wtap|zyd[0-9]+";
};
# Note that the attach/detach with the highest value wins, so that one can
The regex is correct. Changing it is bogus. I need to fix the bug in devd that interprets it incorrectly since the code to protect expansion of variables is interfering with things like this. Created attachment 197291 [details]
skip shell_quote() call for regexes
Indeed, I'm too fast sharing the incomplete analysis/fix as always. Does the attached patch skipping the shell_quote() call for regexes make more sense?
diff --git a/sbin/devd/devd.cc b/sbin/devd/devd.cc
index e81f718159b1..b2d08324511f 100644
--- a/sbin/devd/devd.cc
+++ b/sbin/devd/devd.cc
@@ -666,7 +666,7 @@ config::shell_quote(const string &s)
}
void
-config::expand_one(const char *&src, string &dst)
+config::expand_one(const char *&src, string &dst, bool is_shell)
{
int count;
string buffer;
@@ -705,7 +705,7 @@ config::expand_one(const char *&src, string &dst)
do {
buffer += *src++;
} while (is_id_char(*src));
- dst.append(shell_quote(get_variable(buffer)));
+ dst.append(is_shell ? shell_quote(get_variable(buffer)) : get_variable(buffer));
}
const string
@@ -731,7 +731,7 @@ config::expand_string(const char *src, const char *prepend, const char *append)
}
dst.append(src, var_at - src);
src = var_at;
- expand_one(src, dst);
+ expand_one(src, dst, prepend == NULL);
}
if (append != NULL)
diff --git a/sbin/devd/devd.hh b/sbin/devd/devd.hh
index 534c4d4c5a9d..d770da22226a 100644
--- a/sbin/devd/devd.hh
+++ b/sbin/devd/devd.hh
@@ -172,7 +172,7 @@ protected:
void sort_vector(std::vector<event_proc *> &);
void parse_one_file(const char *fn);
void parse_files_in_dir(const char *dirname);
- void expand_one(const char *&src, std::string &dst);
+ void expand_one(const char *&src, std::string &dst, bool is_shell);
std::string shell_quote(const std::string &s);
bool is_id_char(char) const;
bool chop_var(char *&buffer, char *&lhs, char *&rhs) const;
I think the following is a shorter change that does the right thing...
diff --git a/sbin/devd/devd.cc b/sbin/devd/devd.cc
index e81f718159b1..b2d08324511f 100644
--- a/sbin/devd/devd.cc
+++ b/sbin/devd/devd.cc
@@ -666,7 +666,7 @@ config::shell_quote(const string &s)
}
void
-config::expand_one(const char *&src, string &dst)
+config::expand_one(const char *&src, string &dst, bool is_shell)
{
int count;
string buffer;
@@ -705,7 +705,7 @@ config::expand_one(const char *&src, string &dst)
do {
buffer += *src++;
} while (is_id_char(*src));
- dst.append(shell_quote(get_variable(buffer)));
+ dst.append(is_shell ? shell_quote(get_variable(buffer)) : get_variable(buffer));
}
const string
@@ -731,7 +731,7 @@ config::expand_string(const char *src, const char *prepend, const char *append)
}
dst.append(src, var_at - src);
src = var_at;
- expand_one(src, dst);
+ expand_one(src, dst, prepend == NULL);
}
if (append != NULL)
diff --git a/sbin/devd/devd.hh b/sbin/devd/devd.hh
index 534c4d4c5a9d..d770da22226a 100644
--- a/sbin/devd/devd.hh
+++ b/sbin/devd/devd.hh
@@ -172,7 +172,7 @@ protected:
void sort_vector(std::vector<event_proc *> &);
void parse_one_file(const char *fn);
void parse_files_in_dir(const char *dirname);
- void expand_one(const char *&src, std::string &dst);
+ void expand_one(const char *&src, std::string &dst, bool is_shell);
std::string shell_quote(const std::string &s);
bool is_id_char(char) const;
bool chop_var(char *&buffer, char *&lhs, char *&rhs) const;
looks like I missed the patch before doing my own :) I think Yuri's would work, but mine is simpler. https://reviews.freebsd.org/D17267 has the review, and is where we should continue discussions about the code. A commit references this bug: Author: imp Date: Sat Sep 22 15:32:54 UTC 2018 New revision: 338888 URL: https://svnweb.freebsd.org/changeset/base/338888 Log: We don't need shell protection for when we're expanding matches. Don't add it. This should fix when we do regepx matches against variables we've set and fix wifi bring up. PR: 231441 Approved by: re@ (kib) Differential Revision: https://reviews.freebsd.org/D17267 Changes: head/sbin/devd/devd.cc head/sbin/devd/devd.hh fixed in current, not relevant to -stable. Patch went in and no further reports of breakage. |
