On 2019/7/31 8:09 上午, NeilBrown wrote:
> 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.
>
Hi Neil,
All the above issues are fixed and v2 patches are posted.
> These are all minor cosmetic issues though, so Reviewed-by:
> NeilBrown <neilb@xxxxxxxx>
Thank you for the review :-)
--
Coly Li
