[Fuego] Patch for Functional.check_mounts

[Tim.Bird at sony.com]

> -----Original Message-----
> From: Kumar T [mailto:kumar.hcl.ers.epl at gmail.com]
> 
> Hi Tim,
> 
> Thanks for your comments.
> 
> I have combined the patches as single patch. Please find the attached PATCH
> and source files.
> 
> Thanks,
> Kumar.
>

This code has serious problems.

There was no signed-off-by line in the commit message body.  This is required.

The commit still consists of a patch against previously-submitted code.
I don't have the baseline code to apply this against.

In test_run, when generating a baseline expected_mount file, the
commands used are "mount | awk ...", but this is run on the host,
not on the board.  This should be "cmd mount | awk ..."
And the file should be placed in /fuego-rw/boards/$NODE_NAME/expected_mounts.txt

It has the wrong filename ("expected_mounts_data.txt"), and it's placed
in the log directory.

The Makefile in the fs-test tarball has a target of 'hello'.  This is a
copy-paste bug from the hello_world Makefile.  The target should named
'fs_test'.

The README.md file is empty, except for a comment with no value.

The file fs_test.c has a copyright of "2016 Sony Corporation". This
is clearly a copy-paste bug, and is the wrong copyright.

The routine "read_mountdata()" reads lines from /tmp/file.  I don't
understand how this file would have any mount data.

Can you explain why this approach of using the 'mount' command
is preferred over just reading from /proc/mounts?

You use the 'system' command, which requires a shell, and you call
the 'mount and awk' commands.  It would be much preferable to avoid
using awk on the target.  If you must use 'awk', you should
put an "assert_has_program awk" in function test_precheck().

The routine read_compare_expectedmountdata() generates a PASS result
for every mounted filesystem that was found in the expected data file.
And it generates a FAIL for every mounted filesystem that was not
found in the expected data file.  However, (and this is important), it should
also report FAIL for every expected mount that was not found on mounted
on the machine.

This code needs a lot more work.

The variables EXPECTED_ARR_SIZE and MOUNTED_ARR_SIZE
are not used correctly (or are not named correctly).  You use them to make
a 2-dimensional array.

EXPECTED_ARR_SIZE is only 20 bytes.  The line read from mount data may
b much longer than this.  This should probably be converted to use an
an array of allocated strings.

While is line 74 (starting with line_size =') indented to the same level
as the while loop that follows it.

fs_test.c sometimes uses tabs and sometimes uses spaces.  It should one
or the other, consistently.

Some of these mistakes are basic mistakes in C coding.  I'm afraid that
I cannot debug through basic C issues and fix code that is not suitable
for production use.

After considering the issue, I think it would be better to structure the fs_test.c
code as a tool that takes two files and compares them
(kind of like diff, but allowing out-of-order lines).

Instead of hard-coding the filenames, please use 2 filenames that are passed
on the command line.

If 2 lines are substantially similar (e.g. use the same mount point, or mount device
then they should be shown as referencing the same intended mount, but with different options)

I would also like to structure this code so that it does not use a tarball.  It's very difficult to do code
reviews on the C code when they come in a tarfile.  The usual case with Fuego where a tarball is
used is that the upstream code is not under development, but is assumed to be a finished
product that only requires at most a few patches.  This is not the case here, where the C code
is in development.

I plan to add a feature to Fuego to support an unpack operation for "local" files (source files
that are in the test directory - where the fuego_test.sh file resides).  I'd like to use this test
as a test case for that new feature.  I'll let you know the details of when this feature is completed.

This patch has not been applied.
 -- Tim

> 
> On Wed, 20 Nov 2019 at 01:10, <Tim.Bird at sony.com
> <mailto:Tim.Bird at sony.com> > wrote:
> 
> 
> 
> 
> 	> -----Original Message-----
> 	> From: Kumar T
> 	>
> 	> Hi Tim,
> 	>
> 	>       I am Kumar from HCL Technologies. I am developing test cases
> for
> 	> "Functional.check_mounts" as discussed earlier. Updated the code
> as per
> 	> your review comments.
> 	>
> 	>
> 	> Please find the attached PATCH and other source files. Please
> review.
> 
> 	The patch you sent consists of edits on top of material that is not
> upstream
> 	yet.
> 
> 	You need to combine this work with the previous work and submit it
> 	as one patch.  If the previous patch had been accepted upstream,
> 	this patch consisting of changes would be appropriate.  However,
> 	I need to see the entire test as a single patch in order to review it.
> 
> 	Please use an interactive rebase to combine the commits into a single
> 	commit, and then send that patch.  You can see this blog entry:
> 	https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-
> rewriting-history
> 	for some instructions on how to do a rebase.  See especially the
> section
> 	"Squash commits together".
> 
> 	Thanks,
> 	 -- Tim
> 
> 
> 
> 
> 
>