On 11/19/19 11:54 AM, Stefan Reiter wrote: > On 11/19/19 10:59 AM, Thomas Lamprecht wrote: >> On 11/19/19 10:30 AM, Fabian Grünbichler wrote: >>>> + >>>> # BEGIN implemented abstract methods from PVE::AbstractConfig >>> IMHO, vm_exists_on_node is a prime candidate for being put into >>> AbstractConfig - it's not qemu-specific at all, we probably want to do a >>> similar split for pve-container as well, and we want to use it when >>> appropriate in AbstractConfig. >>> >>> any objections to moving it there? maybe adapt the name? >>> s/vm/guest_config/ ? actual moving could also be done at a latter point >>> to avoid the bump in + depends on pve-guest-common >>> >> >> sounds OK, I'd maybe go for "config_exists_on_node", the guest is >> somewhat implicit for methods coming out of that module.. > > +1 > >> >> Else, as "AbstractConfig->config_file" already can be passed a node, >> it could also be ommitted, just using, for example: >> >> if ! -f AbstractConfig->config_file(...) >> >> over >> >> if ! AbstractConfig->config_exists_on_node(...) >> >> seems not like the biggest win anyway, but no hard feelings... > > I like it because it abstracts the whole 'die' and error message code, a bit > more readable on the call site IMO. Although it might be better named > "check_config_exists_on_node", since without the $noerr (where Fabian was > right, it is unnecessary) the function will always die if there's an error? >
if it always dies, call it "assert_config_ex..", if it does not dies at all I'd just omit it, for a mix I'm not sure, we have quite a pile of short, very specialized methods there, a few can be OK, but not for every specific functionality makes sense, we've pretty crowded interfaces already.. > In general I'd like to leave it in, but should I do so in AbstractConfig or > leave it here for now? not sure, for now I'd probably like the path with the least resistance.. _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
