Fix #150 Extract kernel options from the syslinux.conf file if exist #152

praveenkumar · 2016-11-15T11:45:45Z

@zchee please do a review. I tested with boot2docker, minikube and centos all works as expected. I am still a golang newbie so code might be not so appealing but happy to improve as per suggestion.

Also tested with ubuntu and get error message as expected.

(ubuntu) DBG | Extracting Kernel Options
Error creating machine: Error in driver during machine creation: Not abled to parse isolinux.cfg, Please use --xhyve-boot-cmd option

zchee · 2016-11-15T14:52:13Z

@praveenkumar Thanks! I'll review it :)

zchee · 2016-11-16T22:19:07Z

sorry for the delays.
I'll review at today.

zchee

Finished review. Sorry for the delay.

zchee · 2016-11-17T02:42:39Z

xhyve/xhyve.go

+	return lines
+}
+
+func extractKernelOptions(d *Driver) error {


Could you including this func to Driver method? That's no need d *Driver arg.
like:

func (d *Driver) extractKernelOptions() error {

Or, Is there any reason?

Could you including this func to Driver method?

No, I needed it volumeRootDir := d.ResolveStorePath(isoMountPath) for this, otherwise I have to again need to mount and get those details. Or there any other better I can get this value?

Ah, no, no.
Go language can call d. methods if the own function has a d. pointer receiver.
Yes, call d.ResolveStorePath(isoMountPath) need d, but if change to below, also can call all of d. methods.

func (d *Driver) extractKernelOptions() error {

or, I was misunderstanding...?

@zchee I will check that.

zchee · 2016-11-17T02:43:35Z

xhyve/xhyve.go

@@ -619,6 +665,10 @@ func (d *Driver) extractKernelImages() error {
 		return err
 	}

+	if err := extractKernelOptions(d); err != nil {


Same as d *Driver

zchee · 2016-11-17T02:45:46Z

xhyve/xhyve.go

@@ -397,6 +399,7 @@ func (d *Driver) Create() error {
 		return err
 	}

+


nit: I don't like a double newline.

@zchee Agree I will remove that.

zchee · 2016-11-17T02:58:13Z

xhyve/xhyve.go

+		if err != nil {
+			return err
+		}
+		for _, line := range readIsoLinuxCfg {


I do not know which is better, but that's logic can including readLine function with adding (d *Driver) to readLine.
If you(we) want to find only one append option, Can avoid scan all line on isolinux.cfg with regexp.MatchString and break.

But, It becomes a little complicated and it processing different from the meaning of the readLine function name.

@zchee yes, I will add this logic to readLine function and then return only matched line.

zchee · 2016-11-17T03:16:21Z

xhyve/xhyve.go

@@ -84,6 +85,7 @@ var (
 	ErrMachineNotExist = errors.New("machine does not exist")
 	diskRegexp         = regexp.MustCompile("^/dev/disk([0-9]+)")
 	kernelRegexp       = regexp.MustCompile(`(vmlinu[xz]|bzImage)[\d]*`)
+	kernelOptionRegexp = regexp.MustCompile(`\s*append`)


nit: If there is append text in the menu(or etc) on isolinux.cfg, It might be matched that line.
If append section is always indented, use \s{2}append is better.

But note that \s{2}append regexp pattern still not perfect :(
If there is a possibility that append is not indented, no need to change this pattern.

https://regex101.com/r/LghmGX/2

https://regex101.com/r/ZmgvW4/2

If there is a possibility that append is not indented, no need to change this pattern.

I can't bet of how other distro have it but for Centos/fedora/boot2docker/minikube it's indented I am in favor to keep it as it is in case some distro not follow intended way.

It seems intended.
However, maybe there is append to menu is an edge case.
I leave this to you.

@zchee I will update it to \s{2} in PR.

zchee · 2016-11-17T03:22:35Z

@praveenkumar Ah, forgot @ mention.
This comment just for mentioned.

zchee · 2016-11-17T03:33:43Z

xhyve/xhyve.go

+			}
+		}
+		if d.BootCmd == "" {
+			err = errors.New("Not abled to parse isolinux.cfg, Please use --xhyve-boot-cmd option")


nit: I'm not at good English. but "abled" is google translate suggest Did you mean: Not able to parse isolinux.cfg
https://translate.google.com/?source=gtx_m#en/ja/Not%20abled%20to%20parse%20isolinux.cfg

I leave this to you.

@zchee You are right, need update.

zchee · 2016-11-17T03:45:04Z

@praveenkumar And, nit: comment is not negative. Just wondering(?)
No offense to you.

praveenkumar · 2016-11-17T04:13:08Z

@zchee Thanks for detailed review, I will incorporate those suggestion and update the PR soon.

zchee · 2016-11-17T04:13:44Z

@praveenkumar Thanks :)

… file if exist

zchee · 2016-11-19T13:21:05Z

@praveenkumar thanks for update :) ready?

praveenkumar · 2016-11-20T02:43:48Z

@zchee yes, let's merge it :)

zchee · 2016-11-20T02:45:59Z

@praveenkumar I see :)
I'll merge it after the final check, Thanks!!

praveenkumar · 2016-11-20T03:10:14Z

I'll merge it after the final check.

Sure, take your time 👍

zchee · 2016-11-21T08:39:44Z

@praveenkumar Thanks :) I tested it, works successfully!
LGTM, merge.

zchee requested changes Nov 17, 2016

View reviewed changes

praveenkumar force-pushed the fix_150 branch 2 times, most recently from 67736cb to e1f45c8 Compare November 18, 2016 07:47

Fix machine-drivers#150 Extract kernel options from the syslinux.conf…

e51c8fc

… file if exist

praveenkumar force-pushed the fix_150 branch from e1f45c8 to e51c8fc Compare November 18, 2016 10:05

zchee approved these changes Nov 21, 2016

View reviewed changes

zchee merged commit 261fa05 into machine-drivers:master Nov 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #150 Extract kernel options from the syslinux.conf file if exist #152

Fix #150 Extract kernel options from the syslinux.conf file if exist #152

praveenkumar commented Nov 15, 2016 •

edited

Loading

zchee commented Nov 15, 2016

zchee commented Nov 16, 2016

zchee left a comment

zchee Nov 17, 2016

praveenkumar Nov 18, 2016

zchee Nov 18, 2016

zchee Nov 18, 2016

praveenkumar Nov 18, 2016

zchee Nov 17, 2016

zchee Nov 17, 2016

praveenkumar Nov 17, 2016

zchee Nov 17, 2016 •

edited

Loading

praveenkumar Nov 18, 2016

zchee Nov 17, 2016 •

edited

Loading

praveenkumar Nov 17, 2016

zchee Nov 17, 2016

praveenkumar Nov 18, 2016

zchee commented Nov 17, 2016

zchee Nov 17, 2016

praveenkumar Nov 18, 2016

zchee commented Nov 17, 2016

praveenkumar commented Nov 17, 2016

zchee commented Nov 17, 2016

zchee commented Nov 19, 2016

praveenkumar commented Nov 20, 2016

zchee commented Nov 20, 2016

praveenkumar commented Nov 20, 2016

zchee commented Nov 21, 2016

		@@ -397,6 +399,7 @@ func (d *Driver) Create() error {
		return err
		}

Fix #150 Extract kernel options from the syslinux.conf file if exist #152

Fix #150 Extract kernel options from the syslinux.conf file if exist #152

Conversation

praveenkumar commented Nov 15, 2016 • edited Loading

zchee commented Nov 15, 2016

zchee commented Nov 16, 2016

zchee left a 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

zchee Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zchee Nov 17, 2016 • edited Loading

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

zchee commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zchee commented Nov 17, 2016

praveenkumar commented Nov 17, 2016

zchee commented Nov 17, 2016

zchee commented Nov 19, 2016

praveenkumar commented Nov 20, 2016

zchee commented Nov 20, 2016

praveenkumar commented Nov 20, 2016

zchee commented Nov 21, 2016

praveenkumar commented Nov 15, 2016 •

edited

Loading

zchee Nov 17, 2016 •

edited

Loading

zchee Nov 17, 2016 •

edited

Loading