[pve-devel] [PATCH manager v2 1/2] ui/Parser: add generic functions property_strings

[Thomas Lamprecht]

Am 08/02/2018 um 02:51 PM schrieb Dominik Csapak:
> On 08/02/2018 12:04 PM, Thomas Lamprecht wrote:
>> Am 08/01/2018 um 08:29 PM schrieb Stoiko Ivanov:
>>
>> great for tackling this! some comments inline..
>>
>>> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
>>> ---
>>>   www/manager6/Parser.js | 43
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 43 insertions(+)
>>>
>>> diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
>>> index 13dce766..df2be5dc 100644
>>> --- a/www/manager6/Parser.js
>>> +++ b/www/manager6/Parser.js
>>> @@ -45,6 +45,49 @@ Ext.define('PVE.Parser', { statics: {
>>>              value === 'true';
>>>       },
>>>   +    parsePropertyString: function(value, defaultKey) {
>>> +    var res = {},
>>> +    errors = false;
>>
>> please indent errors here, else it may seem like errors is already
>> defined (e.g., as global variable) a be a bit confusing...
>> Do:
>> var res = {},
>>      errors = false;
>>
>>> +
>>> +    Ext.Array.each(value.split(','), function(p) {
>>> +        var kv = p.split('=', 2);
>>> +        if (Ext.isDefined(kv[1])) {
>>> +        res[kv[0]] = kv[1];
>>> +        } else if (Ext.isDefined(defaultKey)) {
>>> +        if (Ext.isDefined(res[defaultKey])) {
>>> +            errors = true;
>>> +            return false; //break
>>> +        }
>>> +        res[defaultKey] = kv[0];
>>> +        } else {
>>> +        errors = true;
>>> +        return false; // break
>>> +        }
>>> +    });
>>> +
>>> +    if (errors) {
>>> +        return;
>>
>> Hmm, maybe throw an exception? the above cases suggest a situation which
>> continuing with and empty result may not be desired.
>> You may want to omit the errors variable and just throw directly above.
>>
>> throw "there can only be one defaultKey"
>> throw "invalid property string"
>>
>> (as suggestion)
>
> all parsers in Parser.js do this and it does not make sense to throw
> an exception here, because in that case the user never notices
> except that the code stopped working
sounds like quite a big notification that something is not OK ;-)
IMO, a property string has this hard asserts as it get passed from the
backend, if those do not hold it like an BUG_ON in the kernel, and
execution _should_ be stopped.

>
> in the case of parseXXX the caller is responsible to check
> if the returned value is an object or undefined
> and handle a parse error (be it a msg box or something else)
>
What would you show in the message box? Effectively you have to skip
everything you could do there, because you have no valid state, thus
Read Copy Update cannot be done safely, as UI and backend are out of
sync - and that operation is the normal use case for this helper.

at least I'd then save the exception message in error and log it, so
that if an user reports something resulting from this, which can be
really subtle you get a hind in the console...