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

core: Don’t mark TPM device as special #582

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

mz-pdm
Copy link
Member

@mz-pdm mz-pdm commented Aug 10, 2022

TPM device has its own special handling and thus is not a special
device with general properties.

@@ -518,7 +518,7 @@ private VmDevice readOtherHardwareItem(XmlNode node) {
.forValue(String.valueOf(selectSingleNode(node, VMD_TYPE, _xmlNS).innerText));
String device = selectSingleNode(node, VMD_DEVICE, _xmlNS).innerText;
// special devices are treated as managed devices but still have the OTHER OVF ResourceType
managed = VmDeviceCommonUtils.isSpecialDevice(device, type, true);
managed = VmDeviceCommonUtils.isSpecialDevice(device, type, true) || type == VmDeviceGeneralType.TPM;
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nicer to call readManagedVmDevice(node, readDeviceId(node)) when detecting TPM in readHardwareSection so TPM won't be considered as "other hardware" anywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, done.

@smelamud
Copy link
Member

Why this method is in VmDeviceCommonUtils. It is used in OvfReader/OvfWriter only, so it should naturally be in some utility class in org.ovirt.engine.core.utils.ovf package.

TPM device has its own special handling and thus is not a special
device with general properties.  Let’s honor this also by handling TPM
separately from other hardware items.
@mz-pdm
Copy link
Member Author

mz-pdm commented Aug 15, 2022

Why this method is in VmDeviceCommonUtils. It is used in OvfReader/OvfWriter only, so it should naturally be in some utility class in org.ovirt.engine.core.utils.ovf package.

Yes, I wondered about it too, moved there.

import org.ovirt.engine.core.common.businessentities.VmDeviceGeneralType;
import org.ovirt.engine.core.common.utils.VmDeviceType;

public abstract class OvfReaderWriterUtils {
Copy link
Member

Choose a reason for hiding this comment

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

Why making a class abstract, if it contains only static methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I had originally played with the idea to make it a base class and then changed my mind and forgot to update the declarations :-). Done.

/**
* is special device - device which is managed, but contains the general properties
*/
protected static boolean isSpecialDevice(String device, VmDeviceGeneralType type, boolean includeHostDev) {
Copy link
Member

Choose a reason for hiding this comment

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

I think, package access level would be better that protected here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The same, done.

Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

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

I'd extract the changes related to OvfReaderWriterUtils to a different PR - the changes related to avoid marking TPM device as a special device look good

@mz-pdm
Copy link
Member Author

mz-pdm commented Aug 16, 2022

I'd extract the changes related to OvfReaderWriterUtils to a different PR

Done: #587

@mz-pdm
Copy link
Member Author

mz-pdm commented Aug 17, 2022

/ost

@smelamud smelamud merged commit e962af9 into oVirt:master Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants