[PATCH V4 1/2] mtd: add support for partition parsers
Rafał Miłecki
zajec5 at gmail.com
Tue May 23 02:06:37 PDT 2017
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.
More information about the linux-mtd
mailing list