Re: [PATCH V4 1/2] mtd: add support for partition parsers

["Rafał Miłecki" <zajec5@gmail.com>]

On 05/23/2017 10:14 AM, Rafał Miłecki wrote:
> On 05/11/2017 08:21 PM, Brian Norris wrote:
>> On Thu, Mar 30, 2017 at 02:35:26PM +0200, Rafał Miłecki wrote:
>>> + *
>>> + * @slave: part to be parsed for subpartitions
>>> + * @format: partition format used to call a proper parser
>>> + *
>>> + * Some partitions use a specific format to describe contained subpartitions
>>> + * (volumes). This function tries to use a proper parser for a given format and
>>> + * registers found (sub)partitions.
>>> + */
>>> +static int mtd_parse_part(struct mtd_part *slave, const char *format)
>>> +{
>>> +    struct mtd_partitions parsed;
>>> +    const char *probes[2];
>>> +    int i;
>>> +    int err;
>>> +
>>> +    probes[0] = format; /* Use parser with name matching the format */
>>> +    probes[1] = NULL; /* End of parsers */
>>> +    err = parse_mtd_partitions(&slave->mtd, probes, &parsed, NULL);
>>> +    if (err)
>>> +        return err;
>>> +    else if (!parsed.nr_parts)
>>> +        return -ENOENT;
>>> +
>>> +    for (i = 0; i < parsed.nr_parts; i++) {
>>> +        struct mtd_partition *part;
>>> +
>>> +        part = (struct mtd_partition *)&parsed.parts[i];
>>
>> I'm not super-happy with the de-constification cast here. What if the
>> partition array was somehow shared? (I don't expect that, but you're
>> still potentially violating a caller's assumptions when you modify their
>> 'const' data.)
>>
>>> +        part->offset += slave->offset;
>>> +    }
>>> +
>>> +    err = add_mtd_partitions(slave->master, parsed.parts, parsed.nr_parts);
>>
>> Maybe a better way to handle that offset modification is to either pass
>> in the offset to add_mtd_partitions() (e.g., as a simple parameter), or
>> else adapt add_mtd_partitions() to handle receiving a non-master as the
>> first parameter. (Then you just pass "slave", and add_mtd_partitions()
>> handle it with something like "if (mtd_is_partition(...))".)
>>
>> Then I wonder how we want the parenting structure to work; should the
>> sub-partition have the "master" as its parent, or the original
>> partition? I thought Richard had mentioned some problems with the
>> existing parenting structure (with CONFIG_MTD_PARTITIONED_MASTER)
>> already, but I don't remember what those were.
>>
>> Also, if you're "parsing" the slave, but "adding" to the master, then
>> the bounds-checking logic in add_mtd_partitions() won't properly apply,
>> and you'll be able to create sub-partitions that extend beyond the
>> slave, right? If so, then I think (after auditing add_mtd_partitions() a
>> little closer, and possibly adjusting some of its comments) you might
>> really just want to pass 'slave', not 'slave->master'.
>
> I like this idea!

Oh, it's not that simple. Passing struct mtd_part to the add_mtd_partitions
is simple, but it's getting complex afterwards.

1) We can't simply adjust offset in add_mtd_partitions as we are still
    dealing with const struct mtd_partition (note: const).
2) We can't adjust offset after calling allocate_partition as this would
    bypass some validation happening in the allocate_partition.
3) Passing an extra argume to the allocate_partition is a bad idea as it
    already receives uint64_t cur_offset - this would be confusing.