Re-write zipl_helper in C #18

r4f4 · 2017-11-16T13:49:24Z

This is the first step in getting rid of Perl as per #5.

hoeppnerj

Please do not introduce a separate library for dasdview. We already have libdasd, which should be extended as needed (there are actually plans to unify dasd tool related code in the long run with libdasd).

In fact, dasdview_read_attribute() is only used to read the raw_track_access attribute and libdasd already provides a function for that: dasd_sys_raw_track_access().
Please use that function instead and avoid libdasdview.

michael-holzheu

The new util_print_error() function is not required because we use the err()/warn() function family for s390-tools.

michael-holzheu · 2017-11-16T19:34:08Z

libutil/util_base.c

+	va_end (args);
+
+	fprintf(stderr, "Error: %s\n", error_str);
+}


We currently use the err()/warn() family for error printing in the newer s390-tools. See e.g. zconf/chp or zconf/qeth.

So I don't think we need a new libutil function for that.

r4f4 · 2017-11-20T15:26:43Z

I just submitted a new version according to the requested changes.

michael-holzheu · 2017-11-20T19:12:14Z

I just submitted a new version according to the requested changes.

I did a formal review of zipl_helper.device-mapper.c. The code looks already pretty good, thanks! Nevertheless I found some points to beautify the code (according our/my taste) even further:

Sort include files
Use reverse XMAS tree for variables
Don't put return value of function into separate line
Remove inline from functions -> The compiler should handle this
Use consistently "td" for target_data (not e)
Use consistently "major/minor" (not maj/min ,mj/mn, or mmaj, mmin)
Use consistently "target" for "struct target" (not t)
Use consistently "fp" for fopen (not fd) and "fd" for open
Remove superfluous newlines
Some 80 characters line limit fixes
Do not use C++ comments (//) please
Fix macro formatting (tabs in front of "/")
Use the UNUSED macro :-)
Consolidate multi-line variable definitions with same type
Use imperative for comments, e.g. "Return" list instead of "Returns list"
Checkpatch: Don't wrap format strings
Checkpatch: Braces {} are not necessary for single statement blocks
Checkpatch: Block comments use a trailing */ on a separate line
Consistently start comments uppercase

Please review the following patch:

michael-holzheu@43f3573

In case you agree, update your code accordingly please.

FYI: We just started with our coding style document.

michael-holzheu · 2017-11-20T19:17:19Z

Please fix the following compiler warning (with gcc 4.8.5):

CC      zipl/src/zipl_helper.device-mapper.o
zipl_helper.device-mapper.c: In function ‘get_mirror_data’:
zipl_helper.device-mapper.c:502:8: warning: variable ‘token’ set but not used [-Wunused-but-set-variable]
  char *token;
        ^
zipl_helper.device-mapper.c: In function ‘get_multipath_data’:
zipl_helper.device-mapper.c:676:8: warning: variable ‘token’ set but not used [-Wunused-but-set-variable]
  char *token;

michael-holzheu · 2017-11-20T19:19:13Z

If I run the compile a 2nd time, I get the following error:

s390-tools/zipl $ make
...
s390-tools/zipl $ make
make[1]: *** No rule to make target 'chreipl_helper', needed by 'all'.  Stop.
Makefile:5: recipe for target 'all' failed
make: *** [all] Error 2

r4f4 · 2017-11-20T19:20:39Z

Thank you for the review. I'll look into those issues and submit an updated patch shortly.

Great, thanks!

michael-holzheu · 2017-11-21T10:22:55Z

@r4f4 : Thanks for updating your code

michael-holzheu · 2017-11-21T10:23:16Z

@hoeppnerj / @stefan-haberland : Now it's time to do the functional review by our DASD and zipl experts.

michael-holzheu · 2017-11-21T12:32:22Z

@r4f4 : You should add the zipl helper to the .gitignore file as follows:

diff --git a/.gitignore b/.gitignore
index 3c234f6d..9e287803 100644
--- a/.gitignore
+++ b/.gitignore
@@ -86,4 +86,5 @@ zipl/boot/*.exec
 zipl/boot/data.h
 zipl/src/chreipl_helper.device-mapper
 zipl/src/zipl
+zipl/src/zipl_helper.device-mapper
 zkey/zkey

r4f4 · 2017-11-21T14:05:13Z

@michael-holzheu you are right, I forgot about it. I'll submit a fixed version together with any fixes the DASD and zipl experts asked me to do, if you don't mind.

hoeppnerj · 2017-11-21T16:33:55Z

@r4f4 I've had a closer look at the DASD changes, which seem to go beyond the scope of this pull request.

From what I saw you only need a couple of information relevant to the zipl helper: geo information (sectors, heads, cylinders), type (FBA vs ECKD), format (CDL vs LDL), and blocksize.
These information are being obtained by the following IOCTLs: BIODASDINFO (cylinders, type, format), HDIO_GETGEO (sectors, heads), and BLKSSZGET (blocksize).

As we want to grow our libdasd I'd like to suggest to start a new file dasd_ioctl.c which accumulates all IOCTLs starting with the three mentioned ones. Possible function names could be: dasd_get_blocksize(), dasd_get_info(), dasd_get_geo().
The structures you'll need for those (dasd_information2_t and dasd_eckd_characteristics) should then be located in a generic header file dasd_base.h (You may want to have a look at our util library to get an idea how we want to structure these things).

To keep this small, it would be sufficient if you only make use of those new functions in the zipl helper for now and leave the DASD tools untouched. I'd prefer a 2nd pull-request for DASD tool related clean-ups then (You don't have to send one of course, we can and certainly will do it at one point anyway. I'm just trying to keep the scope of this particular pull-request at its minimum).

r4f4 · 2017-11-21T16:53:15Z

@hoeppnerj What about I work on those DASD changes right now in a separate PR while I wait on Zipl guys to comment on my changes?

hoeppnerj · 2017-11-21T17:07:40Z

@r4f4 Sure, feel free, any change is appreciated :-) I just didn't want to make a 2nd PR a prerequisite for the zipl helper. Only wanted to make sure that changes are within a certain scope.

stefan-haberland · 2017-11-27T16:40:10Z

Hi,

thank you very much for your effort. I have tested the helper program in a some setups and it works quite well. Very good.

Unfortunately with SCSI devices and a multipath setup I get the following error:

zipl_helper.device-mapper: ioctl error
Could not retrieve disk information.

The following output is correct but the message itself could lead to some confusion.

Another thing I have noticed is that with a multipath setup and the first path failing the message changed:

original:
Warning: There are one or more failed paths for device 'dm-2'

new message:
zipl_helper.device-mapper: There are one or more failed paths for device 'dm-2'

Again here. A change in the message that is afterwards visible in the zipl tool could lead to some confusion.

Could you please have a look at these points? Thanks.

Regards,
Stefan

r4f4 · 2017-11-27T17:10:59Z

@stefan-haberland Thank you very much for reviewing and trying this patch out. I must remind you that with #19, there will be some changes to this patch regarding the use of DASD ioctls. We can apply the changes in a second commit or wait for the libdasd changes to be merged, whatever you prefer.

Unfortunately with SCSI devices and a multipath setup I get the following error:

zipl_helper.device-mapper: ioctl error
Could not retrieve disk information.

This comes from using dasd_get_info. With the work to libdasd being done in #19, we'll be able to eliminate this warning message.

new message:
zipl_helper.device-mapper: There are one or more failed paths for device 'dm-2'

Again here. A change in the message that is afterwards visible in the zipl tool could lead to some confusion.

This is a result of using the warn/err family of functions. I guess I'll have to drop that and use my own macros to keep the warning messages intact.

r4f4 · 2017-12-15T08:49:58Z

Changes from previous version:

Use libdasd ioctls.
Use util_proc_part_get_entry instead of implementing own function
Add zipl_helper.device-mapper to .gitignore
Don't use warn/err functions to keep output messages intact.
Make sure code conforms to the project's coding style.

stefan-haberland · 2017-12-21T12:05:56Z

Thanks again for the contribution. I did some quick tests and it works pretty good.
But unfortunately I leave for this year. I will have a closer look in January.

r4f4 · 2018-01-09T13:59:16Z

Just a reminder for you guys not to forget this PR :)

stefan-haberland · 2018-01-16T15:41:16Z

Not forgotten. Will have a deeper look.

oberpar · 2018-01-22T13:20:44Z

zipl/src/zipl_helper.device-mapper.c

+		strcpy(name, "LDL");
+		break;
+	default:
+		WARN("Unrecognized dev type %d\n", type);


Set "name" to an empty string here to prevent the potential use of uninitialized strings in the calling function.

oberpar · 2018-01-22T13:22:08Z

zipl/src/zipl_helper.device-mapper.c

+
+	va_start(ap, fmt);
+	util_vasprintf(&cmd, fmt, ap);
+	va_end(ap);


Use util_asprintf to remove the need for va_start() and va_end().

You mean I should util_asprintf the command before passing it to `exec_cmd_and_get_out_stream' function?

I missed that this function itself is receiving varargs.. please ignore this comment.

oberpar · 2018-01-22T13:22:55Z

zipl/src/zipl_helper.device-mapper.c

+		util_list_remove(target->data, td);
+		target_data_free(td);
+	}
+	util_list_free(target->data);


Call target_data_list_free(target->data) to reduce duplicate code.

oberpar · 2018-01-22T13:24:37Z

zipl/src/zipl_helper.device-mapper.c

+
+static int get_dev_characteristics(struct device_characteristics *dc, dev_t dev)
+{
+	char devname[BDEVNAME_SIZE];


devname must be PATH_MAX long or the call to create_temp_device_node could theoretically overrun it.

Good catch.

oberpar · 2018-01-22T13:25:59Z

zipl/src/zipl_helper.device-mapper.c

+		if (res != 0) {
+			ERR("Could not get block size for '%s'\n", devname);
+			goto err;
+		}


Since the call to dasd_get_blocksize() is done in both cases it can be moved outside of the if-clause. Also this can help getting rid of the "res" variable which is only used here.

oberpar · 2018-01-22T13:29:48Z

zipl/src/zipl_helper.device-mapper.c

+		if (strcmp(info.type, "FBA") == 0) {
+			dc->type = DEV_TYPE_FBA;
+			dc->bootsectors = dc->blocksize / SECTOR_SIZE;
+		} else if (strcmp(info.type, "ECKD") == 0) {


Access info.type either via memcmp or strncmp to prevent read access beyond end of type field.

oberpar · 2018-01-22T13:31:19Z

zipl/src/zipl_helper.device-mapper.c

+	struct target *target;
+
+	table = get_table(dev);
+	if (table == NULL || util_list_start(table) == NULL) {


Use util_list_is_empty() instead of util_list_start()==NULL to make the meaning clearer.

oberpar · 2018-01-22T13:32:06Z

zipl/src/zipl_helper.device-mapper.c

+	util_list_remove(table, target);
+	table_free(table);
+
+	target_list = util_list_new(struct target_entry, list);


The return value of util_list_new() must be checked for NULL.

My bad. Not all places use util_malloc.

Yes, using util_malloc() in all library functions should be on our TODO list since its currently somewhat inconsistent (ping @michael-holzheu).

Yes, using util_malloc() in all library functions should be on our TODO list since its currently somewhat inconsistent (ping @michael-holzheu).

Correct - I just submitted the following patch: 89ead06

Ok, I'll remove the NULL checks from my patch.

oberpar · 2018-01-22T13:33:48Z

zipl/src/zipl_helper.device-mapper.c

+	int i;
+
+	for (i = 1; i < argc; i++)
+		cmdline = util_strcat_realloc(cmdline, argv[i]);


Why concatenate all of argv when the checked pattern "%u:%u" would be in argv[1] due to a lack of spaces?

Because it might not. The original script handled cases like "maj:min", "maj :min", "maj : min". So to not break this behavior, I opted to concatenate the possible argvs into one single string and then sscanf that (which then ignores the spaces).
If I can assume the supplied string will always be "maj:min", I'd be happy to drop those lines from the patch.

Actually sscanf() only implicitly ignores spaces in front of %-modifiers, so this would not match "maj :min". and "maj : min".

In any case, the format "major:minor" is documented in the initial source file comment and all s390-tools users (zipl, chreipl) use it in this form, so I would assume it is safe to only parse for that. @michael-holzheu - since you initially wrote that particular portion of the Perl-helper, do you see any reason to keep parsing for spaces around ":" in "major:minor"?

r4f4 · 2018-01-22T14:24:12Z

Thank you for the review, I'll re-submit the patch with fixes soon.

r4f4 · 2018-01-29T19:40:48Z

Code was updated to address @oberpar comments and @michael-holzheu change to util_list.

r4f4 · 2018-02-15T10:51:50Z

Any updates on this?

oberpar · 2018-02-15T12:52:36Z

All my comments have been addressed. @michael-holzheu - how to proceed here?

michael-holzheu · 2018-02-15T17:46:16Z

All my comments have been addressed. @michael-holzheu - how to proceed here?

@r4f4 Could you please update the commit message somehow like the following:

zipl: Rewrite helper script in C

To remove the perl dependency from zipl rewrite the helper script in C.

Signed-off-by: Rafael Fonseca <[email protected]>

@oberpar : Then IMHO we can apply the code.

r4f4 · 2018-02-15T18:03:24Z

@michael-holzheu Done. The new message is much better, tks for the suggestion.

stefan-haberland · 2018-02-16T08:10:33Z

There are 3 warnings regarding unused variables. Could you please have a look?

zipl_helper.device-mapper.c: In function ‘target_free’:
zipl_helper.device-mapper.c:213:27: warning: unused variable ‘n’ [-Wunused-variable]
struct target_data *td, *n;
^
zipl_helper.device-mapper.c:213:22: warning: unused variable ‘td’ [-Wunused-variable]
struct target_data *td, *n;
^
zipl_helper.device-mapper.c: In function ‘get_dev_characteristics’:
zipl_helper.device-mapper.c:330:6: warning: unused variable ‘res’ [-Wunused-variable]
int res; ^

To remove the perl dependency from zipl rewrite the helper script in C. Signed-off-by: Rafael Fonseca <[email protected]>

r4f4 · 2018-02-16T08:38:36Z

@stefan-haberland unused variables were removed.

stefan-haberland · 2018-02-16T08:39:33Z

thanks, I will integrate the code

hoeppnerj reviewed Nov 16, 2017

View reviewed changes

michael-holzheu assigned michael-holzheu and stefan-haberland Nov 16, 2017

michael-holzheu added the enhancement label Nov 16, 2017

michael-holzheu suggested changes Nov 16, 2017

View reviewed changes

r4f4 force-pushed the zipl_helper-c branch from bbe410d to 4fdd3ac Compare November 20, 2017 15:24

michael-holzheu approved these changes Nov 20, 2017

View reviewed changes

r4f4 force-pushed the zipl_helper-c branch from 4fdd3ac to f1e8a0a Compare November 20, 2017 20:30

michael-holzheu assigned hoeppnerj Nov 22, 2017

r4f4 force-pushed the zipl_helper-c branch from f1e8a0a to 5094993 Compare December 15, 2017 08:42

oberpar reviewed Jan 22, 2018

View reviewed changes

r4f4 force-pushed the zipl_helper-c branch 2 times, most recently from bbe410d to 8d50256 Compare January 22, 2018 16:50

r4f4 force-pushed the zipl_helper-c branch from 8d50256 to b783dd8 Compare January 29, 2018 19:39

r4f4 force-pushed the zipl_helper-c branch from b783dd8 to c1206f1 Compare February 15, 2018 18:02

zipl: Rewrite helper script in C

34c4203

To remove the perl dependency from zipl rewrite the helper script in C. Signed-off-by: Rafael Fonseca <[email protected]>

r4f4 force-pushed the zipl_helper-c branch from c1206f1 to 34c4203 Compare February 16, 2018 08:37

stefan-haberland closed this in 599b141 Feb 16, 2018

Re-write zipl_helper in C #18

Re-write zipl_helper in C #18

Conversation

r4f4 commented Nov 16, 2017

hoeppnerj left a comment • edited Loading

Choose a reason for hiding this comment

michael-holzheu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r4f4 commented Nov 20, 2017

michael-holzheu commented Nov 20, 2017 • edited Loading

michael-holzheu commented Nov 20, 2017

michael-holzheu commented Nov 20, 2017

r4f4 commented Nov 20, 2017 • edited by michael-holzheu Loading

michael-holzheu commented Nov 21, 2017

michael-holzheu commented Nov 21, 2017

michael-holzheu commented Nov 21, 2017

r4f4 commented Nov 21, 2017

hoeppnerj commented Nov 21, 2017 • edited Loading

r4f4 commented Nov 21, 2017

hoeppnerj commented Nov 21, 2017

stefan-haberland commented Nov 27, 2017

r4f4 commented Nov 27, 2017

r4f4 commented Dec 15, 2017

stefan-haberland commented Dec 21, 2017

r4f4 commented Jan 9, 2018

stefan-haberland commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r4f4 commented Jan 22, 2018

r4f4 commented Jan 29, 2018

r4f4 commented Feb 15, 2018

oberpar commented Feb 15, 2018

michael-holzheu commented Feb 15, 2018

r4f4 commented Feb 15, 2018

stefan-haberland commented Feb 16, 2018

r4f4 commented Feb 16, 2018

stefan-haberland commented Feb 16, 2018

hoeppnerj left a comment •

edited

Loading

michael-holzheu commented Nov 20, 2017 •

edited

Loading

r4f4 commented Nov 20, 2017 •

edited by michael-holzheu

Loading

hoeppnerj commented Nov 21, 2017 •

edited

Loading