[pve-devel] [PATCH v3 qemu-server 3/8] refactor: split check_running into _exists_ and _running_

[Stefan Reiter]

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?

In general I'd like to leave it in, but should I do so in AbstractConfig 
or leave it here for now?

> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>