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

qmp: Conditionally pass threadID and socketID when CPU device add #84

Merged
merged 1 commit into from
Jan 28, 2019

Conversation

nitkon
Copy link
Contributor

@nitkon nitkon commented Jan 28, 2019

In order to hotplug vCPU on ppc64le, we need not
pass threadID and socketID. So conditionally pass
arguments when executing CPU device add.

Fixes: #83

Signed-off-by: Nitesh Konkar niteshkonkar@in.ibm.com

@nitkon nitkon changed the title qmp: Pass non-empty args only when adding CPU device qmp: Conditionally pass threadID and socketID when CPU device add Jan 28, 2019
@coveralls
Copy link

coveralls commented Jan 28, 2019

Coverage Status

Coverage increased (+0.1%) to 79.896% when pulling 4692f6b on nitkon:master into b9c8f76 on intel:master.

@@ -1154,6 +1154,14 @@ func (q *QMP) ExecuteCPUDeviceAdd(ctx context.Context, driver, cpuID, socketID,
"thread-id": threadID,
}

if socketID != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to remove the initialisation of socket-id and thread-id in the composite literal that initialises the map? Otherwise, this patch doesn't actually do anything, apart from conditionally re-initialise keys that have already been initialised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markdryan: Thanks for the review. Updated the patch.

Copy link
Contributor

@markdryan markdryan left a comment

Choose a reason for hiding this comment

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

Not sure this patch is working as intended.

@markdryan
Copy link
Contributor

Maybe we should update the function documentation as well to mention that socket-id and thread-id are optional on some platforms.

@markdryan
Copy link
Contributor

@devimc Could you take a look at this?

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.

lgtm

thanks @nitkon

@nitkon
Copy link
Contributor Author

nitkon commented Jan 28, 2019

Is the travis-ci because of this PR? I guess not ...

@markdryan
Copy link
Contributor

@nitkon It doesn't look like it. It seems that gometalinter has been updated and has added some new checks. Let me see if I can fix this.

@markdryan
Copy link
Contributor

@nitkon I've submitted a PR that fixes the travis builds #85. When the PR gets merged, you will need to rebase this patch on top of master. This should fix the build for you.

@devimc
Copy link

devimc commented Jan 28, 2019

@nitkon please rebase your pr

For vCPU hotplug to work on ppc64le, we need not
pass threadID and socketID. So conditionally pass
arguments when executing CPU device add.

Fixes: kata-containers#83

Signed-off-by: Nitesh Konkar niteshkonkar@in.ibm.com
@nitkon
Copy link
Contributor Author

nitkon commented Jan 28, 2019

Rebased my PR 15 min back. Is the CI running?

@nitkon
Copy link
Contributor Author

nitkon commented Jan 28, 2019

@markdryan @devimc : All green now.

@devimc devimc merged commit 78d079d into kata-containers:master Jan 28, 2019
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

4 participants