Re: [PATCH 6/7] conf: add hypervisor agnostic, domain start-time, validation function for NetDef

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/14/19 5:20 PM, Cole Robinson wrote:
On 10/22/19 9:24 PM, Laine Stump wrote:
+int
+virDomainNetDefRuntimeValidate(const virDomainNetDef *net G_GNUC_UNUSED)
+{
+    /* Unlike virDomainNetDefValidate(), which is a static function
+     * called internally to this file, virDomainActualNetDefValidate()
+     * is a public function that can be called from a hypervisor after
+     * it has completely setup the NetDef for use by a domain,
+     * including possibly allocating a port from the network driver
+     * (which could change the effective/"actual" type of the NetDef,
+     * thus changing what should/shouldn't be allowed by validation).
+     *

The comment here claims the function is named
virDomainActualNetDefValidate. I think that naming is more consistent
rather than invent a new 'runtime' naming scheme, event though runtime
is more semanticly descriptive. (though TBH I wouldn't mind
s/actual/runtime/ in the codebase anyways, the 'actual' naming always
confuses me)

Well, "actual" is describing *what* it is, and "runtime" is describing *when* it is. In most cases I've been fine with the "actual" name, but this time it didn't seem to adequately convey the usefulness of the function, so I decided to try something else out, and missed the function name in the comment.


Comment should be fixed either way, I'll leave it up to you whether to
actually rename the function.

Thinking about it more, I guess it's better to remain consistent everywhere. Maybe someday someone will find some other adjective that works better than actual in all circumstances (it might even be "runtime", my personal mental jury is still out), and will switch all occurrences at once.

Series:

Reviewed-by: Cole Robinson <crobinso@xxxxxxxxxx>

I think there's a conflict in patch 7, git am gave me some confusing
results, so make sure the code you are moving is incorporating and
possible new changes that landed there since this was posted.

Yeah, I had rebased and was going to resend, but you reviewed the original before I got around to it.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux