Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

qemu: add fw_cfg flag to config #144

Merged
merged 1 commit into from Oct 9, 2020
Merged

Conversation

mazzy89
Copy link
Contributor

@mazzy89 mazzy89 commented Oct 8, 2020

Signed-off-by: Salvatore Mazzarino dev@mazzarino.cz

Append fw_cfg flag

Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

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

thanks @mazzy89 - I left some comments

qemu/qemu.go Outdated Show resolved Hide resolved
qemu/qemu.go Outdated Show resolved Hide resolved
@mazzy89
Copy link
Contributor Author

mazzy89 commented Oct 8, 2020

@devimc makes complete sense. I will make the changes

Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

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

thanks @mazzy89 - I left some comments

qemu/qemu.go Show resolved Hide resolved
qemu/qemu.go Outdated Show resolved Hide resolved
qemu/qemu.go Show resolved Hide resolved
Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

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

thanks @mazzy89

@devimc
Copy link

devimc commented Oct 8, 2020

@jodh-intel please take a look

Copy link
Collaborator

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @mazzy89.

A couple of comments.

qemu/qemu.go Outdated Show resolved Hide resolved
qemu/qemu.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @mazzy89.

lgtm

@mazzy89
Copy link
Contributor Author

mazzy89 commented Oct 9, 2020

@jodh-intel thank you for the fast reaction in picking up this and the nice PR flow.

@jodh-intel
Copy link
Collaborator

np. Thanks very much for your contribution @mazzy89!

Before this lands, please could you squash the commits into a single one please?

@mazzy89
Copy link
Contributor Author

mazzy89 commented Oct 9, 2020

np. Thanks very much for your contribution @mazzy89!

Np. a pleasure. thank you for this neat and useful library.

Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz>
@jodh-intel
Copy link
Collaborator

All green - thanks again @mazzy89!

@jodh-intel jodh-intel merged commit 69f9a50 into kata-containers:master Oct 9, 2020
@mazzy89
Copy link
Contributor Author

mazzy89 commented Oct 10, 2020

@jodh-intel I've noticed that adding the logger to fwcfg now causes the error to be printed out always when fwcfg is empty because the validation func runs always. Is something that we want?

@mazzy89 mazzy89 deleted the fw-cfg branch October 11, 2020 15:46
@jodh-intel
Copy link
Collaborator

@mazzy89 - I think this is a problem. And in fact looking at the fw_cfg option further, this PR isn't quite general enough - QEMU supports >=0 fw_cfg options. That implies Config.FwCfg should be an array/slice. And if it was a slice, Valid() wouldn't be called every time since it would be possible to detect the difference between the following two scenarios (which the current code cannot):

  • no fw_cfg entries specified
  • one invalid fw_cfg specified

If you agree, do you want to raise a follow-on PR for this?

@mazzy89
Copy link
Contributor Author

mazzy89 commented Oct 12, 2020

I have a PR already to fix another issue. So I'm going to modify this in here #150

@jodh-intel
Copy link
Collaborator

Thanks @mazzy89.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants