Re: [PATCH 2/5] dm: Add initial device mapper infrastructure

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 08.09.25 11:27, Tobias Waldekranz wrote:
> On fre, sep 05, 2025 at 18:14, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>> If someone looks at this code, because they have seen this error, they
>> won't necessarily know how to interpret the %d.
> 
> My intention was that it could be correlated with the first column from
> the table generated by dm_asprint()...

Fair enough.

> 
>> Can you either add a comment above the iteration or rename the i to a
>> more telling name?
> 
> ...i.e., it is just an index (i). I'll add a comment describing that.

Ok. I was initially wondering if that's the "level" (e.g. dm-verity on top
of LVM) or something. A comment should be enough.

>> We don't support different block sizes right now and you hardcode
>> SECTOR_SIZE below anyway, but we like to pretend we could, so
>> blk->blockbits would be more apt here.
> 
> I made a conscious decision to use hardcode these everywhere as Linux
> defines that a DM block _is_ 512B. You see these constants all over the
> place in the target implementations in the kernel as well.
> 
> From dmsetup(8):
> | Devices are created by loading a table that specifies a target for
> | each sector (512 bytes) in the logical device.
> 
> So while Barebox might one day support other block sizes, I do not think
> that DM ever will.
> 
> What I also should have done is to have a comment detailing this, above
> the hardcoded block size. If I add this, do you think we should stick
> with the constants, or change to blk->blockbits?

Ok, let's keep it hardcoded then. A single comment in the probe function
is enough.

>>> +char *dm_asprint(struct dm_device *dm)
>>> +{
>>> +	char **strv, *str, *tistr;
>>> +	struct dm_target *ti;
>>> +	int n = 0, strc;
>>> +
>>> +	strc = 1 + dm_n_targets(dm);
>>> +	strv = xmalloc(strc * sizeof(*strv));
>>
>>
>> Meh, maybe we should have a growable string type...
> 
> I have taken a stab at this in v2 with rasprintf(), "the realloc()ing
> sprintf()". Let me know what you think.

Looking forward to it!


Cheers,
Ahmad


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |