On Wed, 20 Nov 2024 14:46:36 +0800 Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote: > From: Yu Kuai <yukuai3@xxxxxxxxxx> > > Because it's marked deprecated for a long time now, and it's not worthy > to support it for new bitmap. > > Now that we don't need to store filename for bitmap, also declare a new > enum type bitmap_type to simplify code. Thanks for the enum! I really appreciate the additional effort you took to make mdadm better. I didn't not review it line by line because I see the problem that must be resolved first. I see that you added BitmapNone and BitmapUnknown and their usage is not clear, let me help you! BitmapUnknown should be used only if we failed to parse bitmap setting in cmdline. Otherwise first and default value should be always BitmapNone because data access is always highest priority and dropping bitmap is always safe. We can print warning in config parse failed or bitmap value is repeated- it is reasonable. If I'm wrong here, please let me know. + It would be nice to add tests to cover these config/cmdline bitmap possibilities to define clear set of expected behavior. It is something already missed so I do not require that strongly from you know. I propose you to create mapping_t for bitmap and to use map_name() to match the bitmap strings, instead of hardcoding them but it is my recommendation not something strongly required. Then, you would be able to remove some checks for both (s->btype != BitmapNone && s->btype != BitmapUnknown). The change proposed by my will provide clear differentiation between error value and set of accepted values, messing that is always confusing for maintainers end readers. I don't see that kind of mess necessary in this case. Thanks, Mariusz
