Skip to content

Conversation

joyent-automation
Copy link

fix live migration

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/5749/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@YanChii commented at 2019-03-18T19:32:27

Uploaded patch set 2: Patch Set 1 was rebased.

@YanChii commented at 2019-04-01T15:04:17

Topic set to live migration

@rmustacc commented at 2019-04-05T14:27:26

Patch Set 2:

(1 comment)

@YanChii commented at 2019-04-06T16:17:00

Uploaded patch set 3: Patch Set 2 was rebased.

@rmustacc commented at 2019-04-08T21:14:25

Patch Set 4:

(1 comment)

@YanChii commented at 2019-04-08T23:13:05

Patch Set 4:

(1 comment)

Patch Set 4 code comments
qemu-kvm-x86.c#517 @rmustacc

Please see my previous comment about how this should be implemented.

qemu-kvm-x86.c#517 @YanChii

Sorry, I missed this. Will fix.
But actually this check is not needed. Correct me if I'm wrong but combination KVM+qemu+SunOS+live_migration will always result in 64-bit OS. This code is not used for any other purpose.

qemu-kvm-x86.c#517 @rmustacc

This, as written, is saying that if qemu is 64-bit, then it's fine. While we don't plan on having a 32-bit qemu, a 32-bit qemu could run on a 64-bit OS. If you want something that's actually the equivalent of the existing Linux check, then we should actually do what I suggested. If we're going to assume that we're only ever 64-bit, then we should set it to 1, and explicitly #error if we're not building 64-bit.

Also, the cstyle of the else statement needs to be fixed here.

qemu-kvm-x86.c#517 @YanChii

Even if never used, I feel the most proper way is to implement SI_ARCHITECTURE_K. Other ways seem too lazy to me.
Pushing the fix.

@rmustacc commented at 2019-04-16T15:09:27

Patch Set 4:

(1 comment)

@YanChii commented at 2019-04-17T11:43:50

Patch Set 5:

(1 comment)

citrus-it pushed a commit to citrus-it/illumos-kvm-cmd that referenced this pull request Jul 3, 2023
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.

2 participants