Mirror Linux lsblk command #1705

ahmedkrmn · 2020-02-23T00:14:05Z

This PR improves the available lsblk command in Embox. It mirrors the linux lsblk command adding the -b, --bytes flags to choose the unit of displaying size.

I tried multiple ways to round the value of size to the nearest integer but they all failed resulting in the error message posted on the issue page. The current behavior is typecasting the float to uint64_t (flooring).

ahmedkrmn · 2020-02-23T00:48:32Z

Travis check was successfully completed on all templates except mips/qemu. This is the error message:

Embox kernel start
	unit: initializing embox.mem.phymem: 
			start=0x894f7000 end=0x90000000 size=112234496
		done
runlevel: init level is 0
	unit: initializing embox.kernel.task.task_resource: done
	unit: initializing embox.mem.static_heap: done
	unit: initializing embox.kernel.task.task_table: done
	unit: initializing embox.kernel.task.kernel_task: done
	unit: initializing embox.profiler.tracing: done
	unit: initializing embox.driver.interrupt.mips_intc: done
	unit: initializing embox.kernel.sched.sched: done
	unit: initializing embox.arch.mips.kernel.exception: done
	unit: initializing embox.arch.mips.kernel.interrupt: done
	unit: initializing embox.driver.clock.mips_clk: done
	unit: initializing embox.kernel.time.timer_handler: done
	unit: initializing embox.kernel.time.kernel_time: done
	test: running embox.test.math.math_test .
	failure at src/tests/math/math_test.c : 45, in function __test_case_at_line_38
		test_assert_true(l == l)
	   case at src/tests/math/math_test.c : 38
		"Test for soft-fp operations"
	
	testing math_test (math/common routines's tests) failed
		1/2 failures
Failed to get into level 1, current level 0

anton-bondarev · 2020-02-23T06:50:27Z

src/cmds/hardware/lsblk/lsblk.c

 #include <inttypes.h>

 #include <drivers/block_dev.h>

+const char *convertUnit(uint64_t *size){


Please, use 'static' if it's an internal routine

anton-bondarev · 2020-02-23T07:17:34Z

src/cmds/hardware/lsblk/lsblk.c

 #include <inttypes.h>

 #include <drivers/block_dev.h>

+const char *convertUnit(uint64_t *size){


use name conversion from Embox code-style
convertUnit->convert_unit

anton-bondarev · 2020-02-23T07:18:15Z

src/cmds/hardware/lsblk/lsblk.c

 int main(int argc, char **argv) {
 	int i;
 	struct block_dev **bdevs;

 	bdevs = get_bdev_tab();
 	assert(bdevs);

+	bool isBytes = argc > 1 && !(strcmp(argv[1], "-b") && strcmp(argv[1], "--bytes"));


name conversion
isBytes->is_bytes

anton-bondarev · 2020-02-23T07:21:20Z

@ahmedkrmn Ahmed, thank you. It's look good, but resolve some my comments please

alexkalmuk · 2020-02-23T07:47:50Z

src/cmds/hardware/lsblk/lsblk.c

 int main(int argc, char **argv) {
 	int i;
 	struct block_dev **bdevs;

 	bdevs = get_bdev_tab();
 	assert(bdevs);

+	bool isBytes = argc > 1 && !(strcmp(argv[1], "-b") && strcmp(argv[1], "--bytes"));


@ahmedkrmn I do not think it's a good idea to have this option at the fixed position. It's much better to analyze the arguments list.

ahmedkrmn · 2020-02-24T11:48:51Z

Thanks @alexkalmuk and @anton-bondarev for the feedback. I will start working on the proposed changes.

ahmedkrmn · 2020-02-28T22:41:33Z

@alexkalmuk, @anton-bondarev
I've applied the coding convention for naming the functions and the variables.
I've also created a separate function to parse the -b and --bytes arguments instead of manually comparing it with the first index.

ahmedkrmn · 2020-02-29T19:10:59Z

The time complexity for the argument vectors parsing function is O(n). It can be implemented in O(logn) if the vector is sorted, but after all, the list of arguments is so small compared to the overhead needed to implement it this way.

anton-bondarev · 2020-02-29T20:39:21Z

src/cmds/hardware/lsblk/lsblk.c

+	return unit;
+}
+
+static bool parse_is_bytes(int argc, char **argv){


Please check Code-style
} else {

anton-bondarev · 2020-02-29T20:40:00Z

src/cmds/hardware/lsblk/lsblk.c

+static bool parse_is_bytes(int argc, char **argv){
+	if (argc == 1) return 0;
+	bool res = 0;
+	for (size_t i = 1; i < argc; i++)


code style
for ( ..) {
,,
}

anton-bondarev · 2020-02-29T20:40:29Z

src/cmds/hardware/lsblk/lsblk.c

+}
+
+static bool parse_is_bytes(int argc, char **argv){
+	if (argc == 1) return 0;


code-style
if (..) {
..
}

anton-bondarev · 2020-02-29T20:49:29Z

The time complexity for the argument vectors parsing function is O(n). It can be implemented in O(logn) if the vector is sorted, but after all, the list of arguments is so small compared to the overhead needed to implement it this way.

Yes, may be. But let start with code-style https://github.com/embox/embox/wiki/Code-Style

alexkalmuk · 2020-03-01T08:18:43Z

@ahmedkrmn Can we just use getopt_* functions to parse arguments?

ahmedkrmn · 2020-03-02T00:28:43Z

@ahmedkrmn Can we just use getopt_* functions to parse arguments?

Didn't know that there already exists a function that parses the argument vector.
I need to find where it is implemented to know how to use it. I only could find where it was used but couldn't reach the implementation.

anton-bondarev · 2020-03-02T08:22:44Z

@ahmedkrmn Can we just use getopt_* functions to parse arguments?

Didn't know that there already exists a function that parses the argument vector.
I need to find where it is implemented to know how to use it. I only could find where it was used but couldn't reach the implementation.

it's declared in getopt.h It's usual and described in linux man (https://linux.die.net/man/3/getopt_long

ahmedkrmn · 2020-03-12T12:29:05Z

@anton-bondarev and @alexkalmuk
I've used the getopt() function defined in getopt.h as requested.
Can you please re-review this?

src/cmds/hardware/lsblk/lsblk.c

anton-bondarev · 2020-03-13T08:28:24Z

@ahmedkrmn Thanks
There are a small code-style mistakes, fix it please.

ahmedkrmn · 2020-03-13T08:52:00Z

@anton-bondarev Can you please point them out?
I followed the styling guide here throughout the whole file.

anton-bondarev · 2020-03-13T09:41:20Z

@anton-bondarev Can you please point them out?
I followed the styling guide here throughout the whole file.

@ahmedkrmn
I commented these places

In part "Tabs and Brackets" said

We use tabs as the indentation

at least in line 46 using spaces at the start of line

And it's not in our code-style

Please move the variable declarations to the start of the block (to the declaration part)

That is enough for me

ahmedkrmn · 2020-03-15T15:39:37Z

@anton-bondarev Done!

anton-bondarev

LGTM

anton-bondarev · 2020-03-16T09:09:31Z

@ahmedkrmn I've approved it. Please rebase under embox/master and squash commits

Add argv parsing and apply naming convention Remove unnecessary printf statement Apply embox code style convention Remove trailing spaces Use for parsing cmd arguments

ahmedkrmn · 2020-03-19T15:05:19Z

@anton-bondarev Done!

anton-bondarev reviewed Feb 23, 2020

View reviewed changes

alexkalmuk reviewed Feb 23, 2020

View reviewed changes

alexkalmuk added cmds Good First Issue labels Feb 23, 2020

alexkalmuk removed the Good First Issue label Feb 26, 2020

anton-bondarev reviewed Feb 29, 2020

View reviewed changes

anton-bondarev reviewed Mar 13, 2020

View reviewed changes

src/cmds/hardware/lsblk/lsblk.c Show resolved Hide resolved

anton-bondarev reviewed Mar 13, 2020

View reviewed changes

src/cmds/hardware/lsblk/lsblk.c Show resolved Hide resolved

anton-bondarev approved these changes Mar 16, 2020

View reviewed changes

Mirror Linux lsblk command

200c239

Add argv parsing and apply naming convention Remove unnecessary printf statement Apply embox code style convention Remove trailing spaces Use for parsing cmd arguments

ahmedkrmn force-pushed the lsblk-unit-conversion branch from 47aefe2 to 200c239 Compare March 19, 2020 15:04

anton-bondarev merged commit 07d861b into embox:master Mar 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mirror Linux lsblk command #1705

Mirror Linux lsblk command #1705

ahmedkrmn commented Feb 23, 2020

ahmedkrmn commented Feb 23, 2020

anton-bondarev Feb 23, 2020

anton-bondarev Feb 23, 2020

anton-bondarev Feb 23, 2020

anton-bondarev commented Feb 23, 2020

alexkalmuk Feb 23, 2020 •

edited

Loading

ahmedkrmn commented Feb 24, 2020

ahmedkrmn commented Feb 28, 2020

ahmedkrmn commented Feb 29, 2020

anton-bondarev Feb 29, 2020

anton-bondarev Feb 29, 2020

anton-bondarev Feb 29, 2020

anton-bondarev commented Feb 29, 2020

alexkalmuk commented Mar 1, 2020

ahmedkrmn commented Mar 2, 2020

anton-bondarev commented Mar 2, 2020

ahmedkrmn commented Mar 12, 2020

anton-bondarev commented Mar 13, 2020

ahmedkrmn commented Mar 13, 2020

anton-bondarev commented Mar 13, 2020

ahmedkrmn commented Mar 15, 2020

anton-bondarev left a comment

anton-bondarev commented Mar 16, 2020

ahmedkrmn commented Mar 19, 2020

Mirror Linux lsblk command #1705

Mirror Linux lsblk command #1705

Conversation

ahmedkrmn commented Feb 23, 2020

ahmedkrmn commented Feb 23, 2020

anton-bondarev Feb 23, 2020

Choose a reason for hiding this comment

anton-bondarev Feb 23, 2020

Choose a reason for hiding this comment

anton-bondarev Feb 23, 2020

Choose a reason for hiding this comment

anton-bondarev commented Feb 23, 2020

alexkalmuk Feb 23, 2020 • edited Loading

Choose a reason for hiding this comment

ahmedkrmn commented Feb 24, 2020

ahmedkrmn commented Feb 28, 2020

ahmedkrmn commented Feb 29, 2020

anton-bondarev Feb 29, 2020

Choose a reason for hiding this comment

anton-bondarev Feb 29, 2020

Choose a reason for hiding this comment

anton-bondarev Feb 29, 2020

Choose a reason for hiding this comment

anton-bondarev commented Feb 29, 2020

alexkalmuk commented Mar 1, 2020

ahmedkrmn commented Mar 2, 2020

anton-bondarev commented Mar 2, 2020

ahmedkrmn commented Mar 12, 2020

anton-bondarev commented Mar 13, 2020

ahmedkrmn commented Mar 13, 2020

anton-bondarev commented Mar 13, 2020

ahmedkrmn commented Mar 15, 2020

anton-bondarev left a comment

Choose a reason for hiding this comment

anton-bondarev commented Mar 16, 2020

ahmedkrmn commented Mar 19, 2020

alexkalmuk Feb 23, 2020 •

edited

Loading