Yes, and you don't want to support two ways to do something if it hasn't been noticed in ten years.
> On Oct 9, 2020, at 11:37 , Gedare Bloom <ged...@rtems.org> wrote:
>
> Excellent analysis. I would like Chris to weigh in, but the rationale
> to remove instead of try to fix makes sense because it is basically an
> undocumented shell API.
>
> On Fri, Oct 9, 2020 at 7:38 AM Frank Kuehndel
> <frank.kuehn...@embedded-brains.de> wrote:
>>
>> The shell has an 'fdisk' command which has sub-commands 'mount' and
>> 'unmount'.
>> These two sub-commands have a bug which causes them to be not able
>> to mount anything. This proposed patch removes the buggy file
>> cpukit/libblock/src/bdpart-mount.c and the mount/unmount commands
>> from 'fdisk' as bug fix. The 'fdisk' command itself is not removed.
>> The reasons for removing the sub-commands (instead of fixing the issue) are:
>>
>> 1) The bug has been introduced on 2010-May-31 with commit
>> 29e92b090c8bc35745aa5c89231ce806bcb11e57. Since ten years no one
>> can use this feature, nor has anybody complained about it.
>>
>> 2) Besides of the 'fdisk' 'mount' sub-command, the shell has the
>> usual 'mount' and 'unmount' commands which can serve as
>> substitutes.
>>
>> 3) There are additional minor issues (see further down) which needed to
>> be addressed when the file will be kept.
>>
>> What follows below is the precise bug description.
>>
>> The bug is in function rtems_bdpart_mount() which is only be used
>> by the 'fdisk' shell command to mount all partitions of a disk with a
>> single command:
>>
>>> fdisk DISK_NAME mount
>>> mounts the file system of each partition of the disk
>>>
>>> fdisk DISK_NAME unmount
>>> unmounts the file system of each partition of the disk
>>
>> The whole command does not work because in file
>> cpukit/libblock/src/bdpart-mount.c line 103 specifies the file system type
>> of each partition to be "msdos". Yet, "msdos" does not exist. The name
>> must be "dosfs".
>>
>> Beside of this fundamental problem, there are more issues with the code
>> in bdpart-mount.c:
>>
>> 1) The function returns RTEMS_SUCCESSFUL despite the mount always fails.
>>
>> 2) The reason for errors is not written to the terminal.
>>
>> 3) The directory '/mnt' is created but not deleted later on (failure or
>> not).
>>
>> 3) There is no documentation about this special 'fdisk' feature in the
>> RTEMS Shell Guide ('fdisk' is mentioned but its documentation is a
>> bit short):
>> https://docs.rtems.org/branches/master/shell/
>> file_and_directory.html#fdisk-format-disk
>>
>> 4) Only "msdos" formatted partitions can be mounted and all partitions
>> are mounted read-only. This is hard coded and cannot be changed by
>> options. Moreover, there is no information about this to the user of
>> the shell (i.e. using 'fdisk' mount requires insider knowledge).
>>
>> How to reproduce:
>>
>> 1) For testing, I use the 'testsuites/samples/fileio.exe' sample with qemu:
>>
>>> cd rtems
>>> env QEMU_AUDIO_DRV="none" qemu-system-arm -net none -nographic \
>>> -M realview-pbx-a9 -m 256M -kernel \
>>> build/arm/realview_pbx_a9_qemu/testsuites/samples/fileio.exe
>>
>> 2) Type any key to stop the timer and enter the sample tool.
>> Type 's' to enter the shell, login as 'root' with the password
>> shown in the terminal.
>>
>> 3) Type the following shell commands (they create a RAM disk,
>> partition it, register it, format it and try to mount it):
>>
>>> mkrd
>>> fdisk /dev/rda fat32 16 write mbr
>>> fdisk /dev/rda register
>>> mkdos /dev/rda1
>>> fdisk /dev/rda mount
>>
>> 4) The last line above is the command which fails - without an error
>> message. There exists a '/mnt' directory but no '/mnt/rda1' directory
>> as it should be:
>>
>>> ls -la /mnt
>>
>> 5) If you change line 103 of 'cpukit/libblock/src/bdpart-mount.c'
>> from "msdos" to "dosfs", compile and build the executable and
>> re-run the above test, '/mnt/rda1' exists (but the file system
>> is mounted read-only).
>>
>> Close #4131
>> ---
>> cpukit/Makefile.am | 1 -
>> cpukit/include/rtems/bdpart.h | 35 +-----
>> cpukit/libblock/src/bdpart-mount.c | 184 -----------------------------
>> cpukit/libmisc/shell/fdisk.c | 36 +-----
>> spec/build/cpukit/librtemscpu.yml | 1 -
>> 5 files changed, 7 insertions(+), 250 deletions(-)
>> delete mode 100644 cpukit/libblock/src/bdpart-mount.c
>>
>> diff --git a/cpukit/Makefile.am b/cpukit/Makefile.am
>> index 1f9124e175..bdd8ebef33 100644
>> --- a/cpukit/Makefile.am
>> +++ b/cpukit/Makefile.am
>> @@ -44,7 +44,6 @@ librtemscpu_a_SOURCES += dtc/libfdt/fdt_wip.c
>> librtemscpu_a_SOURCES += libblock/src/bdbuf.c
>> librtemscpu_a_SOURCES += libblock/src/bdpart-create.c
>> librtemscpu_a_SOURCES += libblock/src/bdpart-dump.c
>> -librtemscpu_a_SOURCES += libblock/src/bdpart-mount.c
>> librtemscpu_a_SOURCES += libblock/src/bdpart-read.c
>> librtemscpu_a_SOURCES += libblock/src/bdpart-register.c
>> librtemscpu_a_SOURCES += libblock/src/bdpart-sort.c
>> diff --git a/cpukit/include/rtems/bdpart.h b/cpukit/include/rtems/bdpart.h
>> index 8886c3614d..dde716e146 100644
>> --- a/cpukit/include/rtems/bdpart.h
>> +++ b/cpukit/include/rtems/bdpart.h
>> @@ -7,10 +7,10 @@
>> */
>>
>> /*
>> - * Copyright (c) 2009-2012 embedded brains GmbH. All rights reserved.
>> + * Copyright (c) 2009-2012, 2020 embedded brains GmbH. All rights reserved.
>> *
>> * embedded brains GmbH
>> - * Obere Lagerstr. 30
>> + * Dornierstr. 4
>> * 82178 Puchheim
>> * Germany
>> * <rt...@embedded-brains.de>
>> @@ -302,37 +302,6 @@ rtems_status_code rtems_bdpart_unregister(
>> size_t count
>> );
>>
>> -/**
>> - * @brief Mounts all supported file systems inside the logical disks derived
>> - * from the partitions of the physical disk device with name @a disk_name.
>> - *
>> - * For each partition in the partition table @a partitions with @a count
>> - * partitions it will be checked if it contains a supported file system. In
>> - * this case a mount point derived from the disk name will be created in the
>> - * mount base path @a mount_base. The file system will be mounted there.
>> The
>> - * partition number equals the partition table index plus one. The mount
>> point
>> - * name for each partition will be the concatenation of the mount base path,
>> - * the disk device file name and the parition number.
>> - *
>> - * @see rtems_bdpart_read().
>> - */
>> -rtems_status_code rtems_bdpart_mount(
>> - const char *disk_name,
>> - const rtems_bdpart_partition *partitions,
>> - size_t count,
>> - const char *mount_base
>> -);
>> -
>> -/**
>> - * @brief Unmounts all file systems mounted with rtems_bdpart_mount().
>> - */
>> -rtems_status_code rtems_bdpart_unmount(
>> - const char *disk_name,
>> - const rtems_bdpart_partition *partitions,
>> - size_t count,
>> - const char *mount_base
>> -);
>> -
>> /**
>> * @brief Prints the partition table @a partitions with @a count partitions
>> to
>> * standard output.
>> diff --git a/cpukit/libblock/src/bdpart-mount.c
>> b/cpukit/libblock/src/bdpart-mount.c
>> deleted file mode 100644
>> index cfc08ead30..0000000000
>> --- a/cpukit/libblock/src/bdpart-mount.c
>> +++ /dev/null
>> @@ -1,184 +0,0 @@
>> -/**
>> - * @file
>> - *
>> - * @ingroup rtems_bdpart
>> - *
>> - * @brief Block Device Partition Management
>> - */
>> -
>> -/*
>> - * Copyright (c) 2009, 2010
>> - * embedded brains GmbH
>> - * Obere Lagerstr. 30
>> - * D-82178 Puchheim
>> - * Germany
>> - * <rt...@embedded-brains.de>
>> - *
>> - * The license and distribution terms for this file may be
>> - * found in the file LICENSE in this distribution or at
>> - * http://www.rtems.org/license/LICENSE.
>> - */
>> -
>> -#ifdef HAVE_CONFIG_H
>> -#include "config.h"
>> -#endif
>> -
>> -#include <stdio.h>
>> -#include <stdlib.h>
>> -#include <string.h>
>> -
>> -#include <rtems.h>
>> -#include <rtems/bdpart.h>
>> -#include <rtems/libio.h>
>> -
>> -rtems_status_code rtems_bdpart_mount(
>> - const char *disk_name,
>> - const rtems_bdpart_partition *pt RTEMS_UNUSED,
>> - size_t count,
>> - const char *mount_base
>> -)
>> -{
>> - rtems_status_code esc = RTEMS_SUCCESSFUL;
>> - const char *disk_file_name = strrchr( disk_name, '/');
>> - char *logical_disk_name = NULL;
>> - char *logical_disk_marker = NULL;
>> - char *mount_point = NULL;
>> - char *mount_marker = NULL;
>> - size_t disk_file_name_size = 0;
>> - size_t disk_name_size = strlen( disk_name);
>> - size_t mount_base_size = strlen( mount_base);
>> - size_t i = 0;
>> -
>> - /* Create logical disk name base */
>> - logical_disk_name = malloc( disk_name_size + RTEMS_BDPART_NUMBER_SIZE);
>> - if (logical_disk_name == NULL) {
>> - return RTEMS_NO_MEMORY;
>> - }
>> - strncpy( logical_disk_name, disk_name, disk_name_size);
>> -
>> - /* Get disk file name */
>> - if (disk_file_name != NULL) {
>> - disk_file_name += 1;
>> - disk_file_name_size = strlen( disk_file_name);
>> - } else {
>> - disk_file_name = disk_name;
>> - disk_file_name_size = disk_name_size;
>> - }
>> -
>> - /* Create mount point base */
>> - mount_point = malloc( mount_base_size + 1 + disk_file_name_size +
>> RTEMS_BDPART_NUMBER_SIZE);
>> - if (mount_point == NULL) {
>> - esc = RTEMS_NO_MEMORY;
>> - goto cleanup;
>> - }
>> - strncpy( mount_point, mount_base, mount_base_size);
>> - mount_point [mount_base_size] = '/';
>> - strncpy( mount_point + mount_base_size + 1, disk_file_name,
>> disk_file_name_size);
>> -
>> - /* Markers */
>> - logical_disk_marker = logical_disk_name + disk_name_size;
>> - mount_marker = mount_point + mount_base_size + 1 + disk_file_name_size;
>> -
>> - /* Mount supported file systems for each partition */
>> - for (i = 0; i < count; ++i) {
>> - /* Create logical disk name */
>> - int rv = snprintf( logical_disk_marker, RTEMS_BDPART_NUMBER_SIZE,
>> "%zu", i + 1);
>> - if (rv >= RTEMS_BDPART_NUMBER_SIZE) {
>> - esc = RTEMS_INVALID_NAME;
>> - goto cleanup;
>> - }
>> -
>> - /* Create mount point */
>> - strncpy( mount_marker, logical_disk_marker, RTEMS_BDPART_NUMBER_SIZE);
>> - rv = rtems_mkdir( mount_point, S_IRWXU | S_IRWXG | S_IRWXO);
>> - if (rv != 0) {
>> - esc = RTEMS_IO_ERROR;
>> - goto cleanup;
>> - }
>> -
>> - /* Mount */
>> - rv = mount(
>> - logical_disk_name,
>> - mount_point,
>> - "msdos",
>> - 0,
>> - NULL
>> - );
>> - if (rv != 0) {
>> - rmdir( mount_point);
>> - }
>> - }
>> -
>> -cleanup:
>> -
>> - free( logical_disk_name);
>> - free( mount_point);
>> -
>> - return esc;
>> -}
>> -
>> -rtems_status_code rtems_bdpart_unmount(
>> - const char *disk_name,
>> - const rtems_bdpart_partition *pt RTEMS_UNUSED,
>> - size_t count,
>> - const char *mount_base
>> -)
>> -{
>> - rtems_status_code esc = RTEMS_SUCCESSFUL;
>> - const char *disk_file_name = strrchr( disk_name, '/');
>> - char *mount_point = NULL;
>> - char *mount_marker = NULL;
>> - size_t disk_file_name_size = 0;
>> - size_t disk_name_size = strlen( disk_name);
>> - size_t mount_base_size = strlen( mount_base);
>> - size_t i = 0;
>> -
>> - /* Get disk file name */
>> - if (disk_file_name != NULL) {
>> - disk_file_name += 1;
>> - disk_file_name_size = strlen( disk_file_name);
>> - } else {
>> - disk_file_name = disk_name;
>> - disk_file_name_size = disk_name_size;
>> - }
>> -
>> - /* Create mount point base */
>> - mount_point = malloc( mount_base_size + 1 + disk_file_name_size +
>> RTEMS_BDPART_NUMBER_SIZE);
>> - if (mount_point == NULL) {
>> - esc = RTEMS_NO_MEMORY;
>> - goto cleanup;
>> - }
>> - strncpy( mount_point, mount_base, mount_base_size);
>> - mount_point [mount_base_size] = '/';
>> - strncpy( mount_point + mount_base_size + 1, disk_file_name,
>> disk_file_name_size);
>> -
>> - /* Marker */
>> - mount_marker = mount_point + mount_base_size + 1 + disk_file_name_size;
>> -
>> - /* Mount supported file systems for each partition */
>> - for (i = 0; i < count; ++i) {
>> - /* Create mount point */
>> - int rv = snprintf( mount_marker, RTEMS_BDPART_NUMBER_SIZE, "%zu", i +
>> 1);
>> - if (rv >= RTEMS_BDPART_NUMBER_SIZE) {
>> - esc = RTEMS_INVALID_NAME;
>> - goto cleanup;
>> - }
>> -
>> - /* Unmount */
>> - rv = unmount( mount_point);
>> - if (rv == 0) {
>> - /* Remove mount point */
>> - rv = rmdir( mount_point);
>> - if (rv != 0) {
>> - esc = RTEMS_IO_ERROR;
>> - goto cleanup;
>> - }
>> - }
>> - }
>> -
>> -cleanup:
>> -
>> - free( mount_point);
>> -
>> - return esc;
>> -}
>> diff --git a/cpukit/libmisc/shell/fdisk.c b/cpukit/libmisc/shell/fdisk.c
>> index d071e4fbba..c244922492 100644
>> --- a/cpukit/libmisc/shell/fdisk.c
>> +++ b/cpukit/libmisc/shell/fdisk.c
>> @@ -5,10 +5,10 @@
>> */
>>
>> /*
>> - * Copyright (c) 2009
>> + * Copyright (c) 2009, 2020
>> * embedded brains GmbH
>> - * Obere Lagerstr. 30
>> - * D-82178 Puchheim
>> + * Dornierstr. 4
>> + * 82178 Puchheim
>> * Germany
>> * <rt...@embedded-brains.de>
>> *
>> @@ -61,13 +61,8 @@ static const char rtems_bdpart_shell_usage [] =
>> "\tcreates a logical disk for each partition of the disk\n"
>> "\n"
>> "fdisk DISK_NAME unregister\n"
>> - "\tdeletes the logical disks associated with the partitions of the disk\n"
>> - "\n"
>> - "fdisk DISK_NAME mount\n"
>> - "\tmounts the file system of each partition of the disk\n"
>> - "\n"
>> - "fdisk DISK_NAME unmount\n"
>> - "\tunmounts the file system of each partition of the disk\n"
>> + "\tdeletes the logical disks associated with the partitions\n"
>> + "\tof the disk\n"
>> "\n"
>> "option values:\n"
>> "\tDISK_NAME: absolute path to disk device like '/dev/hda'\n"
>> @@ -84,14 +79,11 @@ static int rtems_bdpart_shell_main( int argc, char
>> **argv)
>> unsigned dist [RTEMS_BDPART_PARTITION_NUMBER_HINT];
>> size_t count = RTEMS_BDPART_PARTITION_NUMBER_HINT;
>> const char *disk_name = NULL;
>> - const char *mount_base = "/mnt";
>> bool do_create = false;
>> bool do_read = false;
>> bool do_write = false;
>> bool do_register = false;
>> bool do_unregister = false;
>> - bool do_mount = false;
>> - bool do_unmount = false;
>> bool do_dump = false;
>>
>> if (argc < 2) {
>> @@ -112,12 +104,6 @@ static int rtems_bdpart_shell_main( int argc, char
>> **argv)
>> } else if (strcmp( argv [2], "unregister") == 0) {
>> do_read = true;
>> do_unregister = true;
>> - } else if (strcmp( argv [2], "mount") == 0) {
>> - do_read = true;
>> - do_mount = true;
>> - } else if (strcmp( argv [2], "unmount") == 0) {
>> - do_read = true;
>> - do_unmount = true;
>> } else {
>> RTEMS_BDPART_SHELL_ERROR( "unexpected option: %s", argv [2]);
>> }
>> @@ -250,18 +236,6 @@ static int rtems_bdpart_shell_main( int argc, char
>> **argv)
>> RTEMS_BDPART_SHELL_ERROR_SC( sc, "cannot unregister partitions of '%s'",
>> disk_name);
>> }
>>
>> - if (do_mount) {
>> - /* Mount partitions */
>> - sc = rtems_bdpart_mount( disk_name, pt, count, mount_base);
>> - RTEMS_BDPART_SHELL_ERROR_SC( sc, "cannot mount partitions of '%s' to
>> '%s'", disk_name, mount_base);
>> - }
>> -
>> - if (do_unmount) {
>> - /* Unmount partitions */
>> - sc = rtems_bdpart_unmount( disk_name, pt, count, mount_base);
>> - RTEMS_BDPART_SHELL_ERROR_SC( sc, "cannot unmount partitions of '%s'",
>> disk_name);
>> - }
>> -
>> if (do_dump) {
>> /* Dump partitions */
>> rtems_bdpart_dump( pt, count);
>> diff --git a/spec/build/cpukit/librtemscpu.yml
>> b/spec/build/cpukit/librtemscpu.yml
>> index fe440de738..d039bd6267 100644
>> --- a/spec/build/cpukit/librtemscpu.yml
>> +++ b/spec/build/cpukit/librtemscpu.yml
>> @@ -539,7 +539,6 @@ source:
>> - cpukit/libblock/src/bdbuf.c
>> - cpukit/libblock/src/bdpart-create.c
>> - cpukit/libblock/src/bdpart-dump.c
>> -- cpukit/libblock/src/bdpart-mount.c
>> - cpukit/libblock/src/bdpart-read.c
>> - cpukit/libblock/src/bdpart-register.c
>> - cpukit/libblock/src/bdpart-sort.c
>> --
>> 2.26.2
>>
>> _______________________________________________
>> devel mailing list
>> devel@rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
> _______________________________________________
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
Peter
-----------------
Peter Dufault
HD Associates, Inc. Software and System Engineering
This email is delivered through the public internet using protocols subject to
interception and tampering.
signature.asc
Description: Message signed with OpenPGP
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
