[U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support

[Stephen Warren <swarren@wwwdotorg.org>]

On 09/14/2015 11:35 AM, Hans de Goede wrote:
> Hi,
>
> On 01-09-15 22:03, Stephen Warren wrote:
>> On 08/22/2015 11:04 AM, Hans de Goede wrote:
>>> Add generic fs support, so that commands like ls, load and test -e
>>> can be
>>> used on ubifs.
>>
>>> @@ -530,6 +531,28 @@ int get_device_and_partition(const char *ifname,
>>> const char *dev_part_str,
>>>           return 0;
>>>       }
>>>
>>> +#ifdef CONFIG_CMD_UBIFS
>>> +    /*
>>> +     * Special-case ubi, ubi goes through a mtd, rathen then through
>>> +     * a regular block device.
>>> +     */
>>> +    if (0 == strcmp(ifname, "ubi")) {
>>> +        if (!ubifs_mounted) {
>>> +            printf("UBIFS not mounted, use ubifsmount to mount
>>> volume first!\n");
>>> +            return -1;
>>> +        }
>>> +
>>> +        *dev_desc = NULL;
>>> +        memset(info, 0, sizeof(*info));
>>> +        strcpy((char *)info->type, BOOT_PART_TYPE);
>>> +        strcpy((char *)info->name, "UBI");
>>> +#ifdef CONFIG_PARTITION_UUIDS
>>> +        info->uuid[0] = 0;
>>> +#endif
>>> +        return 0;
>>> +    }
>>> +#endif
>>
>> We now have two paths through this function that can "Return" a NULL
>> dev_desc. This makes it impossible for sandbox and ubifs to successfully
>> co-exist in the same U-Boot binary, since the sandbox and ubifs fs probe
>> functions won't be able to tell if "hostfs" or "ubifs" was passed to
>> get_device_and_partition(). Perhaps there's no ubifs support in sandbox
>> right now, so there's no issue?
>
> Right, ubifs is for raw nand, sandbox is for access to a host filesystem
> in a sandbox build. I basically never expect both CONFIG_CMD_UBIFS and
> CONFIG_SANDBOX to be set at the same time. I'll add a pre-processor
> check + #error to enforce this in the next version.
>
>> If this is an issue that needs to be solved now, I think the best
>> solution would be for the two special cases in
>> get_device_and_partition() to "return" a real dev_desc rather than NULL.
>> Since there's nothing meaningful to put there, how about returning a
>> hard-coded value that can then be checked in the fs probe functions to
>> make sure it matches:
>>
>> get_device_and_partition():
>>
>>     if (hostfs) {
>>         ...
>>         *dev_desc = &hostfs_fake_dev_desc;
>>         ...
>>         return 0;
>>     }
>>     if (ubi) {
>>         ...
>>         *dev_desc = &ubifs_fake_dev_desc;
>>         ...
>>         return 0;
>>     }
>>
>> ubifs_set_blk_dev():
>>
>>     if (rbdd != &ubifs_fake_dev_desc)
>>         return -1;
>>     ...
>>     return 0;
>>
>> ... that said, I wonder if the ubifs special case in
>> get_device_and_partition() shouldn't actually perform the ubifs_mount()
>> call itself, based on the user-supplied parameters?
>
> That is not possible as the supplied parameter for a generic fs call
> is a device index + partition number, where as ubi volumes use
> names (strings).

I don't think that matters.

The "generic fs" plumbing added in this series doesn't require a volume 
name to be passed from the "generic fs" layer to the ubifs layer; it 
assumes that a ubifs volume is already mounted and so no volume identity 
is required. I wasn't implying that should be changed. Given that, the 
disparity between parameters doesn't matter, since there's no need to 
translate the ubifs_fake_dev_dec to a ubifs volume name at all; the only 
thing it'd be used for is as an identity check to dispatch from the 
generic fs layer to the right underlying filesystem.

Still, the current solution you have should be fine for now. We only 
need to fix this if someone implements a raw NAND emulator for sandbox.