| Message ID | 167867897868.11443.7240557073570592164.stgit@noble.brown (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Jes Sorensen |
| Headers | show |
| Series | Assorted patches relating to mdmon | expand |
On 3/12/23 23:42, NeilBrown wrote: > Factor out IMSM_NO_PLATFORM testing into a single function that caches > the result. > > Allow mdmon to explicitly set the result to "1" so that we don't need > the ENV var in the unit file > > Check if the kernel command line contains "mdadm.imsm.test=1" and in > that case assert NO_PLATFORM. This simplifies testing in a virtual > machine. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > mdadm.h | 2 ++ > mdmon.c | 6 ++++++ > super-intel.c | 43 ++++++++++++++++++++++++++++++++++++++++--- > systemd/mdmon@.service | 3 --- > 4 files changed, 48 insertions(+), 6 deletions(-) Hi Neil > diff --git a/super-intel.c b/super-intel.c > index e155a8ae99cb..a514dea6f95c 100644 > --- a/super-intel.c > +++ b/super-intel.c > @@ -629,6 +629,43 @@ static const char *_sys_dev_type[] = { > [SYS_DEV_VMD] = "VMD" > }; > > +static int no_platform = -1; > + > +static int check_no_platform(void) > +{ > + static const char search[] = "mdadm.imsm.test=1"; > + int fd; > + char buf[1024]; This isn't safe, /proc/cmdline can be longer than 1024 characters. Cheers, Jes
On Mon, 20 Mar 2023, Jes Sorensen wrote: > On 3/12/23 23:42, NeilBrown wrote: > > Factor out IMSM_NO_PLATFORM testing into a single function that caches > > the result. > > > > Allow mdmon to explicitly set the result to "1" so that we don't need > > the ENV var in the unit file > > > > Check if the kernel command line contains "mdadm.imsm.test=1" and in > > that case assert NO_PLATFORM. This simplifies testing in a virtual > > machine. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > mdadm.h | 2 ++ > > mdmon.c | 6 ++++++ > > super-intel.c | 43 ++++++++++++++++++++++++++++++++++++++++--- > > systemd/mdmon@.service | 3 --- > > 4 files changed, 48 insertions(+), 6 deletions(-) > > Hi Neil > > > diff --git a/super-intel.c b/super-intel.c > > index e155a8ae99cb..a514dea6f95c 100644 > > --- a/super-intel.c > > +++ b/super-intel.c > > @@ -629,6 +629,43 @@ static const char *_sys_dev_type[] = { > > [SYS_DEV_VMD] = "VMD" > > }; > > > > +static int no_platform = -1; > > + > > +static int check_no_platform(void) > > +{ > > + static const char search[] = "mdadm.imsm.test=1"; > > + int fd; > > + char buf[1024]; > > This isn't safe, /proc/cmdline can be longer than 1024 characters. Why not safe? I agree that /proc/cmdline can be longer than 1024 but the only only considers the first 1023 at most, and does so safely. What would you prefer? Should I use conf_line() to read the line and split it into words for us? Thanks, NeilBrown > > Cheers, > Jes > >
On 3/19/23 18:05, NeilBrown wrote: > On Mon, 20 Mar 2023, Jes Sorensen wrote: >> On 3/12/23 23:42, NeilBrown wrote: >>> Factor out IMSM_NO_PLATFORM testing into a single function that caches >>> the result. >>> >>> Allow mdmon to explicitly set the result to "1" so that we don't need >>> the ENV var in the unit file >>> >>> Check if the kernel command line contains "mdadm.imsm.test=1" and in >>> that case assert NO_PLATFORM. This simplifies testing in a virtual >>> machine. >>> >>> Signed-off-by: NeilBrown <neilb@suse.de> >>> --- >>> mdadm.h | 2 ++ >>> mdmon.c | 6 ++++++ >>> super-intel.c | 43 ++++++++++++++++++++++++++++++++++++++++--- >>> systemd/mdmon@.service | 3 --- >>> 4 files changed, 48 insertions(+), 6 deletions(-) >> >> Hi Neil >> >>> diff --git a/super-intel.c b/super-intel.c >>> index e155a8ae99cb..a514dea6f95c 100644 >>> --- a/super-intel.c >>> +++ b/super-intel.c >>> @@ -629,6 +629,43 @@ static const char *_sys_dev_type[] = { >>> [SYS_DEV_VMD] = "VMD" >>> }; >>> >>> +static int no_platform = -1; >>> + >>> +static int check_no_platform(void) >>> +{ >>> + static const char search[] = "mdadm.imsm.test=1"; >>> + int fd; >>> + char buf[1024]; >> >> This isn't safe, /proc/cmdline can be longer than 1024 characters. > > Why not safe? I agree that /proc/cmdline can be longer than 1024 but the > only only considers the first 1023 at most, and does so safely. > > What would you prefer? Should I use conf_line() to read the line and > split it into words for us? Hi Neil, Not safe was probably not the right word. I just saw your new patch, I think that's much better. My concern is that it failing to parse in some conditions is going to be a pain in the neck to debug. Thanks, Jes >> >> >
diff --git a/mdadm.h b/mdadm.h index 1674ce1307b2..16acb2dd1ce4 100644 --- a/mdadm.h +++ b/mdadm.h @@ -1258,6 +1258,8 @@ extern struct superswitch super0, super1; extern struct superswitch super_imsm, super_ddf; extern struct superswitch mbr, gpt; +void imsm_set_no_platform(int v); + struct metadata_update { int len; char *buf; diff --git a/mdmon.c b/mdmon.c index 60ba318253b9..f557e12c6533 100644 --- a/mdmon.c +++ b/mdmon.c @@ -318,6 +318,12 @@ int main(int argc, char *argv[]) {NULL, 0, NULL, 0} }; + /* + * mdmon should never complain due to lack of a platform, + * that is mdadm's job if at all. + */ + imsm_set_no_platform(1); + while ((opt = getopt_long(argc, argv, "thaF", options, NULL)) != -1) { switch (opt) { case 'a': diff --git a/super-intel.c b/super-intel.c index e155a8ae99cb..a514dea6f95c 100644 --- a/super-intel.c +++ b/super-intel.c @@ -629,6 +629,43 @@ static const char *_sys_dev_type[] = { [SYS_DEV_VMD] = "VMD" }; +static int no_platform = -1; + +static int check_no_platform(void) +{ + static const char search[] = "mdadm.imsm.test=1"; + int fd; + char buf[1024]; + int n = 0; + + if (no_platform >= 0) + return no_platform; + + if (check_env("IMSM_NO_PLATFORM")) { + no_platform = 1; + return 1; + } + fd = open("/proc/cmdline", O_RDONLY); + if (fd >= 0) { + n = read(fd, buf, sizeof(buf)-1); + close(fd); + } + if (n >= (int)sizeof(search)) { + buf[n] = 0; + if (strstr(buf, search) != NULL) { + no_platform = 1; + return 1; + } + } + no_platform = 0; + return 0; +} + +void imsm_set_no_platform(int v) +{ + no_platform = v; +} + const char *get_sys_dev_type(enum sys_dev_type type) { if (type >= SYS_DEV_MAX) @@ -2699,7 +2736,7 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle int result=1; if (enumerate_only) { - if (check_env("IMSM_NO_PLATFORM")) + if (check_no_platform()) return 0; list = find_intel_devices(); if (!list) @@ -4722,7 +4759,7 @@ static int find_intel_hba_capability(int fd, struct intel_super *super, char *de devname); return 1; } - if (!is_fd_valid(fd) || check_env("IMSM_NO_PLATFORM")) { + if (!is_fd_valid(fd) || check_no_platform()) { super->orom = NULL; super->hba = NULL; return 0; @@ -10697,7 +10734,7 @@ static int imsm_get_allowed_degradation(int level, int raid_disks, ******************************************************************************/ int validate_container_imsm(struct mdinfo *info) { - if (check_env("IMSM_NO_PLATFORM")) + if (check_no_platform()) return 0; struct sys_dev *idev; diff --git a/systemd/mdmon@.service b/systemd/mdmon@.service index cb6482d9ff29..e7ee17a8bf91 100644 --- a/systemd/mdmon@.service +++ b/systemd/mdmon@.service @@ -12,9 +12,6 @@ Before=initrd-switch-root.target Documentation=man:mdmon(8) [Service] -# mdmon should never complain due to lack of a platform, -# that is mdadm's job if at all. -Environment=IMSM_NO_PLATFORM=1 # The mdmon starting in the initramfs (with dracut at least) # cannot see sysfs after root is mounted, so we will have to # 'takeover'. As the '--offroot --takeover' don't hurt when
Factor out IMSM_NO_PLATFORM testing into a single function that caches the result. Allow mdmon to explicitly set the result to "1" so that we don't need the ENV var in the unit file Check if the kernel command line contains "mdadm.imsm.test=1" and in that case assert NO_PLATFORM. This simplifies testing in a virtual machine. Signed-off-by: NeilBrown <neilb@suse.de> --- mdadm.h | 2 ++ mdmon.c | 6 ++++++ super-intel.c | 43 ++++++++++++++++++++++++++++++++++++++++--- systemd/mdmon@.service | 3 --- 4 files changed, 48 insertions(+), 6 deletions(-)