On Wed, Jul 31 2019, Coly Li wrote:
> When people assemble a md raid device with a large number of
> component deivces (e.g. 1500 DASD disks), the raid device detail
> information generated by 'mdadm --detail --export $devnode' is very
> large. It is because the detail information contains information of
> all the component disks (even the missing/failed ones).
>
> In such condition, when udev-md-raid-arrays.rules is triggered and
> internally calls "mdadm --detail --no-devices --export $devnode",
> user may observe systemd error message ""invalid message length". It
> is because the following on-stack raw message buffer in systemd code
> is not big enough,
> systemd/src/libudev/libudev-monitor.c
> _public_ struct udev_device *udev_monito ...
> struct ucred *cred;
> union {
> struct udev_monitor_netlink_header nlh;
> char raw[8192];
> } buf;
> Even change size of raw[] from 8KB to larger size, it may still be not
> enough for detail message of a md raid device with much larger number of
> component devices.
>
> To fix this problem, an extra option '--no-devices' is added (the
> original idea is proposed by Neil Brown). When printing detailed
> information of a md raid device, if '--no-devices' is specified, then
> all component devices information will not be printed, then the output
> message size can be restricted to a small number, even with the systemd
> only has 8KB on-disk raw buffer, the md raid array udev rules can work
> correctly without failure message.
>
> Signed-off-by: Coly Li <colyli@xxxxxxx>
> Cc: Neil Brown <neilb@xxxxxxxx>
> ---
> Detail.c | 17 +++++++++++++----
> ReadMe.c | 1 +
> mdadm.c | 4 ++++
> mdadm.h | 2 ++
> 4 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/Detail.c b/Detail.c
> index 20ea03a..879ca3b 100644
> --- a/Detail.c
> +++ b/Detail.c
> @@ -56,7 +56,7 @@ int Detail(char *dev, struct context *c)
> */
> int fd = open(dev, O_RDONLY);
> mdu_array_info_t array;
> - mdu_disk_info_t *disks;
> + mdu_disk_info_t *disks = NULL;
> int next;
> int d;
> time_t atime;
> @@ -280,7 +280,8 @@ int Detail(char *dev, struct context *c)
> }
> map_free(map);
> }
> - if (sra) {
> +
> + if (!c->no_devices && sra) {
> struct mdinfo *mdi;
> for (mdi = sra->devs; mdi; mdi = mdi->next) {
> char *path;
> @@ -654,18 +655,23 @@ This is pretty boring
> closedir(dir);
> printf("\n\n");
> }
> + }
>
> + if (!c->no_devices && !c->brief) {
> if (array.raid_disks)
> printf(" Number Major Minor RaidDevice State\n");
> else
> printf(" Number Major Minor RaidDevice\n");
I would prefer this if statement say inside the 'else' branch above.
That does mean it needs to be indented more, but I think it is worth the
cost.
> }
> - free(info);
>
> for (d = 0; d < max_disks * 2; d++) {
> char *dv;
> mdu_disk_info_t disk = disks[d];
>
> + /* if --no_devices specified, quit devices iteration loop */
> + if (c->no_devices)
> + break;
Again, I would put this outside the loop, even though it mean more indents.
> +
> if (d >= array.raid_disks * 2 &&
> disk.major == 0 && disk.minor == 0)
> continue;
> @@ -766,8 +772,11 @@ This is pretty boring
> !enough(array.level, array.raid_disks, array.layout, 1, avail))
> rv = 2;
>
> - free(disks);
> out:
> + if (disks)
> + free(disks);
> + if (info)
> + free(info);
free is documented as "If ptr is NULL, no operation is performed". So
just
free(disks);
free(info);
is sufficient.
These are all minor cosmetic issues though, so
Reviewed-by: NeilBrown <neilb@xxxxxxxx>
Thanks,
NeilBrown
> close(fd);
> free(subarray);
> free(avail);
> diff --git a/ReadMe.c b/ReadMe.c
> index 12ccf83..eaf1042 100644
> --- a/ReadMe.c
> +++ b/ReadMe.c
> @@ -181,6 +181,7 @@ struct option long_options[] = {
>
> /* For Detail/Examine */
> {"brief", 0, 0, Brief},
> + {"no-devices",0, 0, NoDevices},
> {"export", 0, 0, 'Y'},
> {"sparc2.2", 0, 0, Sparc22},
> {"test", 0, 0, 't'},
> diff --git a/mdadm.c b/mdadm.c
> index 25a1abd..1fb8086 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -159,6 +159,10 @@ int main(int argc, char *argv[])
> c.brief = 1;
> continue;
>
> + case NoDevices:
> + c.no_devices = 1;
> + continue;
> +
> case 'Y': c.export++;
> continue;
>
> diff --git a/mdadm.h b/mdadm.h
> index c36d7fd..e96f271 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -442,6 +442,7 @@ enum special_options {
> NoSharing,
> HelpOptions,
> Brief,
> + NoDevices,
> ManageOpt,
> Add,
> AddSpare,
> @@ -552,6 +553,7 @@ struct context {
> int runstop;
> int verbose;
> int brief;
> + int no_devices;
> int force;
> char *homehost;
> int require_homehost;
> --
> 2.16.4
Attachment:
signature.asc
Description: PGP signature
