Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add device-remove and device-remove-all option #325

Merged
merged 3 commits into from
Apr 12, 2017

Conversation

zhouhao3
Copy link

Signed-off-by: zhouhao [email protected]

@@ -60,6 +60,14 @@ read the configuration from `config.json`.
The *fileMode*, *uid*, *gid* are optional.
*fileMode* is the file mode of the device file.
*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...]*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@zhouhao3 zhouhao3 Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zhouhao3 zhouhao3 force-pushed the device-dle branch 4 times, most recently from 1f2936e to 1ee61c0 Compare February 15, 2017 03:20
Devices: []rspec.Device{},
Devices: []rspec.Device{
{
Path: "/dev/fuse",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@zhouhao3 zhouhao3 force-pushed the device-dle branch 6 times, most recently from 60dc4e1 to 66ed818 Compare February 16, 2017 06:38
@@ -642,33 +660,33 @@ var deviceType = map[string]bool{
"p": true, // a FIFO
}

// 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--device -> --device-add

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

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"},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be cli.BoolFlag

}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Mashimiao
Copy link

generate.go needs gofmt

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.")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, PTAL

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

Mashimiao commented Feb 28, 2017

LGTM

Approved with PullApprove

@zhouhao3
Copy link
Author

zhouhao3 commented Mar 9, 2017

@mrunalp @liangchenye PTAL

@zhouhao3
Copy link
Author

zhouhao3 commented Apr 6, 2017

@mrunalp @vbatts @hqhq @crosbymichael PTAL

zhouhao added 3 commits April 12, 2017 16:45
@zhouhao3
Copy link
Author

@liangchenye
Copy link
Member

liangchenye commented Apr 12, 2017

LGTM

Approved with PullApprove

@liangchenye
Copy link
Member

liangchenye commented Apr 12, 2017

#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
Copy link

Mashimiao commented Apr 12, 2017

LGTM

Approved with PullApprove

@Mashimiao Mashimiao merged commit 18a122b into opencontainers:master Apr 12, 2017
@zhouhao3 zhouhao3 deleted the device-dle branch April 13, 2017 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants