On 09.03.21 11:08, Fabian Ebner wrote:
> On 09.03.21 08:17, Thomas Lamprecht wrote:
>> On 08.03.21 13:26, Fabian Ebner wrote:
>>
>> is this covered by some tests already?
>>
>
> Haven't seen any. I can try and add some tests that mirror the config-related
> behavior of the restore_XYZ_archive functions (directly testing those doesn't
> seem feasible to me at the moment, lots of PBS/VMA+pipes interaction...)
Yeah, I'd mostly just do unit testing on sanitize_restored_config directly,
although if more can be covered without a bigger extra cost that'd be naturally
great, but not a must.
>>> + my $oldvalue = $oldconf->{$key};
>>> +
>>> + if (defined($oldvalue) &&
$oldvalue eq $value) {
>>
>> To work a bit better this would need to normalize values for comparison, as
>> for
>> example, one can have a boolean option as '1' or 'on', IIRC, and format
>> strings
>> may have changed order, so the equal check may fail even if they are
>> semantically equal.
>>
>> Not sure how often this can occur in practice as we control the outgoing
>> parser,
>> especially as this only matters for $rootonlyoptions, but did you
>> thought/checked
>> this already?
>>
>
> I did briefly think about it, but decided it wasn't worth the extra
> complexity of parsing and deeply comparing everything. It's not only the
> $rootonlyoptions though, e.g.
> usb0: host=1-1,usb3=1
> and
> usb0: usb3=1,host=1-1
> would also be affected.
>
> For containers, we don't even look at the current values at all, but always
> drop the 'lxc' options for a non-root user restore. But if you think
it's worth it, I can try and add that.
hmm, if the output of the config is somewhat stable then I', ok with simple
for now..
>>> + $res .= "$line\n";
>>> + next;
>>> + }
>>> +
>>> + if ($rootonlyoptions->{$key} ||
>>> + (is_valid_drivename($key)
&& $is_bad_drive->($key, $value)) ||
>>> + ($key =~ m/^serial\d+$/
&& $value ne 'socket') ||
>>> + ($key =~ m/^usb\d+$/ &&
$value !~ m/spice/) ||
>>> + $key =~ m/^parallel\d+$/ ||
>>> + $key =~ m/^hostpci\d+$/) {
>>> + warn "WARNING: SKIPPING CONFIGURATION LINE '$line'. " .
>>> + "Restore as root to include it.\n";
>>
>> I'd tone down the capslock a bit, eg.:
>>
>> warn "WARN: skip restoring line '$line' due to privilege restrictions"
>> ." - restore as root to include it\n"
>>
>> ideally we set the warning count for tasks in the restore worker, like
PBS does, so that
>> this shows up in the gui as "orange" task with some warnings in the task
>> list.
>>
>
> I wasn't aware of this feature. I suppose we should do that for the skipped
> options for LXC too then.
>
It's relatively new and introduced in PBS only, so there may be some catch up
work to do
in pve-common, but GUI support should be universal.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
