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

[Thomas Lamprecht]

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..