QEMU_VM: Add nic_extra_param_types support

[heywji]

Add nic_extra_param_types params support for nic_extra_params to optimize virtio-net QEMU cmdline type handling.

ID: 3269

Signed-off-by: wji [email protected]

[heywji]

@leidwang Hi,

Could you please help review this netkvm qemu_vm improvement?

I have tested positive and negative logic and kept the original logic that defaults to str. All of that works great for me.

[heywji]

@vivianQizhu Hi, I'm calling you to review this patch as well. Thanks a lot.

[leidwang]

Suggested change

	converted_val = value_type(val)
	converted_val = eval(value_type)(val)

Current code is wrong, it can not convert the val to expected type, and it will raise an error.Please have a check.

[leidwang]

>>> nic_extra_params_type = {"a":"int"}
>>> value_type = nic_extra_params_type.get("a")
>>> value_type
'int'
>>> val = "10"
>>> converted_val = value_type(val)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'str' object is not callable
>>> converted_val = eval(value_type)(val)
>>> type(converted_val)
<class 'int'>

[heywji]

Hi @leidwang
We have a split opinion on line 1. Currently, I am using it in this way:

>>> nic_extra_params_type = {"a":int}
>>> value_type = nic_extra_params_type.get("a")
>>> value_type
<class 'int'>
>>> val = "10"
>>> converted_val = value_type(val)
>>> type(converted_val)
<class 'int'>
>>> 

[leidwang]

nic_extra_params_type = {"a":int}
"int" is not a number, it's a string, I think we need add "" for it.

[heywji]

After discussing with @leidwang offline, we want to let @vivianQizhu decide.

In my opinion, Any data can be put into a dict of Python class type because all memory is mapped at the System Bottom. I don't think there is a need for" to set the class type.

Leidong's opinion is that he saw some "int"(highlight: "") in our avocado-vt framework and tp-qemu project. We should make the style the same.

We are just waiting for your professional opinion. @vivianQizhu

[leidwang]

1. In a dict, if the value of a key is "int", it is actually no different from "abc".I t is just a string, in this case, we need add "" for it. I think a better format is nic_extra_params_type = {"a": "int"}
   2.Regardless of which writing style we adopt, we should consider how to handle the other style as well, in order to avoid errors caused by inconsistent writing.

[heywji]

Hi @leidwang,

I accepted your advice about this patch after talking with @vivianQizhu offline.
I will update the code and test it. I will let you review it again when everything is ready.

Thank you~

[heywji]

@leidwang, call you to review this patch again. Thanks in advance.

[leidwang]

LGTM.

[heywji]

@vivianQizhu please help tp review it when you are free, thanks.

[PaulYuuu]

I know you want to fix the issue for param type in newer qemu versions, however this should be fixed in #4056.

Each parameter has a unique type and users should not modify it based on their idea.

[vivianQizhu]

@PaulYuuu Nice patch for #4056, but I don't find how it handle extra_params, it only handles named parameters, I suppose this PR won't be conflict with that one, as extra_params can only be string now, we need to extend it.

[vivianQizhu]

@PaulYuuu Correct myself, I see how it handles all params, with _update_args_type_from_qemu().
@heywji Could you try with #4056 and see if it fixes your issue? If so I guess we can close this one.

[heywji]

@vivianQizhu @PaulYuuu OK, I will try it.

[PaulYuuu]

@PaulYuuu Correct myself, I see how it handles all params, with _update_args_type_from_qemu().

Yes, it should cover all parameters, also it supports both nic_extra_params and netdev_extra_params. The PR has not yet collected all correct parameter types, it should require all feature owners for review.