add device-remove and device-remove-all option by zhouhao3 · Pull Request #325 · opencontainers/runtime-tools

zhouhao3 · 2017-02-14T10:06:02Z

Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

mrunalp · 2017-02-14T18:18:00Z

man/oci-runtime-tool-generate.1.md

    *uid*/*gid* is the user/group id of the device file.
+  This option can be specified multiple times.
+
+**--device-remove**=*TYPE:MAJOR:MINOR:PATH[:OPTIONS...]*


type:major:minor:path should be enough to identify the device. There is not need to parse the options.

I think it will be better to add, if only these optional fields are not the same, then will it delete the wrong device?

I use the whole structure of the comparison, so I think every field is needed.

wking · 2017-02-15T05:03:15Z

generate/generate.go

-			Devices: []rspec.Device{},
+			Devices: []rspec.Device{
+				{
+					Path:  "/dev/fuse",


New() is supposed to generate a config that the runtime-tools maintainers feel is a good default (without knowing anything about the container itself). I don't think that config should contain/dev/fuse unless the user explicitly asks for it.

Here I want to add a default value to the devices, so that you can better achieve devices-remove and other functions.
You mean to give the assignment value is not good, or value /dev/fuse is not good?

There's no need to add a value to the default config just so you can test device removal. Test that by using a template config (although we don't have a test suite for the generator at the moment, see #180 for a stub). But the criterion for adding an entry to New() should be “is this a useful part of a generic config?”, and I don't think /dev/fuse passes that bar.

Mashimiao · 2017-02-28T05:15:23Z

cmd/oci-runtime-tool/generate.go


-// addDevice takes the raw string passed with the --device flag, parses it, and add it
-func addDevice(device string, g *generate.Generator) error {
+// parseDevice takes the raw string passed with the --device flag


--device -> --device-add

Mashimiao · 2017-02-28T07:02:52Z

cmd/oci-runtime-tool/generate.go

-	cli.StringSliceFlag{Name: "device", Usage: "specifies a device which must be made available in the container"},
+	cli.StringSliceFlag{Name: "device-add", Usage: "add a device which must be made available in the container"},
+	cli.StringSliceFlag{Name: "device-remove", Usage: "remove a device which must be made available in the container"},
+	cli.StringSliceFlag{Name: "device-remove-all", Usage: "remove all devices which must be made available in the container"},


should be cli.BoolFlag

Mashimiao · 2017-02-28T07:03:54Z

cmd/oci-runtime-tool/generate.go

+		}
+	}
+
+	if context.IsSet("device-remove-all") {


change to context.Bool("device-remove-all")

Mashimiao · 2017-02-28T07:27:42Z

generate.go needs gofmt

Mashimiao · 2017-02-28T07:46:39Z

generate/generate.go

+			return
+		}
+		if dev.Type == device.Type && dev.Major == device.Major && dev.Minor == device.Minor {
+			fmt.Println("WARNING: The same type, major and minor should not be used for multiple devices.")


I'm afraid this will break default output. Maybe fmt.Fprintln(os.Stderr, "") will be better

updated, PTAL

Mashimiao · 2017-02-28T08:24:41Z

LGTM

zhouhao3 · 2017-03-09T02:12:23Z

@mrunalp @liangchenye PTAL

zhouhao3 · 2017-04-06T02:38:39Z

@mrunalp @vbatts @hqhq @crosbymichael PTAL

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

zhouhao3 · 2017-04-12T08:52:40Z

@vbatts @mrunalp @liangchenye @Mashimiao PTAL

liangchenye · 2017-04-12T09:33:53Z

LGTM

liangchenye · 2017-04-12T09:35:43Z

#343

I agree with @Kxuan , we need to have add clear prefix of Linux* (and others) config/functions.
But I think we can do it after this commit and within one PR.

Mashimiao · 2017-04-12T09:37:06Z

LGTM

mrunalp reviewed Feb 14, 2017

View reviewed changes

zhouhao3 force-pushed the device-dle branch 4 times, most recently from 1f2936e to 1ee61c0 Compare February 15, 2017 03:20

wking reviewed Feb 15, 2017

View reviewed changes

zhouhao3 force-pushed the device-dle branch 6 times, most recently from 60dc4e1 to 66ed818 Compare February 16, 2017 06:38

Mashimiao reviewed Feb 28, 2017

View reviewed changes

zhouhao3 force-pushed the device-dle branch from 76743c8 to 7e1b6ef Compare February 28, 2017 07:16

Mashimiao reviewed Feb 28, 2017

View reviewed changes

zhouhao3 force-pushed the device-dle branch 2 times, most recently from 1a4edce to 80b1053 Compare February 28, 2017 08:14

zhouhao added 3 commits April 12, 2017 16:45

perfect device-add option

71bccea

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

add device-remove option

261a736

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

add device-remove-all option

6e253af

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

zhouhao3 force-pushed the device-dle branch from 80b1053 to 6e253af Compare April 12, 2017 08:49

Mashimiao merged commit 18a122b into opencontainers:master Apr 12, 2017

zhouhao3 deleted the device-dle branch April 13, 2017 00:54

Conversation

zhouhao3 commented Feb 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouhao3 Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mashimiao commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mashimiao commented Feb 28, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouhao3 commented Mar 9, 2017

Uh oh!

zhouhao3 commented Apr 6, 2017

Uh oh!

zhouhao3 commented Apr 12, 2017

Uh oh!

liangchenye commented Apr 12, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liangchenye commented Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mashimiao commented Apr 12, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zhouhao3 Feb 15, 2017 •

edited

Loading

Mashimiao commented Feb 28, 2017 •

edited by caniszczyk

Loading

liangchenye commented Apr 12, 2017 •

edited by caniszczyk

Loading

liangchenye commented Apr 12, 2017 •

edited

Loading

Mashimiao commented Apr 12, 2017 •

edited by caniszczyk

Loading