[WIP DO NOT MERGE] ZnapZend.pm::killThemAll() : do not dereference empty backupSets that…

[jimklimov]

… is not an array type

[jimklimov]

@oetiker : This one seems good: no not ok and done_ hits found in Travis build logs :)

[coveralls]

Coverage decreased (-0.2%) to 91.451% when pulling 8c93e396ad952e34c50360cdc5e200bedc7c842d on jimklimov:no-deref-string-as-array into 4551defc759d71c6aea73d3aa6984a5372acdee5 on oetiker:master.

[jimklimov]

I've seen the error when doing other tests, added a long sleep() to see interesting data better, and then Ctrl+C'ed out of it. The program exited with a message that it can not dereference an array from string "0".

My guess\gut-feeling now is that this may be a downside of the support for string-or-array processing of backupSet plans. When there is nothing to return (e.g. looking for dataset names absent in the system), I think it might return a zero (numeric IIRC) as a sort of false.

On Fri, Nov 22, 2019, 19:49 Tobias Oetiker notifications@github.com wrote:

@oetiker commented on this pull request.

In lib/ZnapZend.pm https://github.com/oetiker/znapzend/pull/460#discussion_r349742255:

@@ -111,16 +111,19 @@ my $killThemAll = sub {

 Mojo::IOLoop->reset;

• for my $backupSet (@{$self->backupSets}){
• kill (SIGTERM, $backupSet->{snap_pid}) if $backupSet->{snap_pid};
• kill (SIGTERM, $backupSet->{send_pid}) if $backupSet->{send_pid};
• }
• sleep 1;
• for my $backupSet (@{$self->backupSets}){
• waitpid($backupSet->{snap_pid}, WNOHANG)
• || kill(SIGKILL, $backupSet->{snap_pid}) if $backupSet->{snap_pid};
• waitpid($backupSet->{send_pid}, WNOHANG)
• || kill(SIGKILL, $backupSet->{send_pid}) if $backupSet->{send_pid};

• Avoid "dereferencing" a value and type of $self->backupSets that is not an array object

• if (defined($self->backupSets) && $self->backupSets ne '0' && $self->backupSets != 0) {

this is equivalent to if ($self->backupSets) { in what situation would there be a string '0' be returned ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/oetiker/znapzend/pull/460?email_source=notifications&email_token=AAMPTFGHZAWSII76ASDQRQTQVASUTA5CNFSM4JQTYP32YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMWV5MY#pullrequestreview-321740467, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMPTFFF453O47ZDUTEZECDQVASUTANCNFSM4JQTYP3Q .

[oetiker]

so my suggested fix would take care of that problem too, right ?

[jimklimov]

Probably so, did not have the time then to look up proper syntax for this, was running off from work. Thanks :)

[jimklimov]

Hmm, by coverage it seems that tests for shutdown of process are lacking.

[jimklimov]

Ok... let's hit this area with a big hammer now...

[jimklimov]

No not ok and done_ faults in travis logs at the moment; the not-covered lines are mostly changed indentation in that exit handler that is originally fully not covered, and a bit of this new ARRAYness sanity check.

[oetiker]

aargh, sorry this patch does not sit well with me ... I think we should not have to be guessing wether backupSets points to an array ... rather if there is a case where some part of the code puts something else into theat property, that place should get fixed ...

do you know where it is happening ?

[jimklimov]

I agree this ended up uglier than intended... I think the problem was introduced here

https://github.com/oetiker/znapzend/pull/460/files#diff-1a78a43e932b58340e5099daab573df3R209 and https://github.com/oetiker/znapzend/pull/460/files#diff-1a78a43e932b58340e5099daab573df3R224

In hindsight, probably not for a good reason, the code was iterated until error-handling worked and grew iteratively just like a crooked bonzai tree :)

At least, now that it is relatively clear where the empty-array cases need to be handled as errors, the focus can be moved for callers to do so more explicitly. I wonder if there is also some way or rather need to use exceptions here. A month later, can't really remember what the huge idea was with the false returns here. I think I was experimenting with ways to differentiate finding no matches (no datasets with backup plan in the requested scope) vs. getting zfs errors (bad dataset name etc.) and was looking for ways to propagate the reason "why we got 0 answers". But not sure now that really matters, beside maybe setting an exit code as erroneous or not.

[oetiker]

fixing the case where the content ends up to ba a space instead of a pointer to an empty array would be a worthwhile endevor ... can you reproduce it ?

[jimklimov]

Hello, sorry for a delay - I'm in the midst of migrating to a new computer and the usual end-of-year hassle. I'll try to get to this PR (or rather a new one with "proper fix"), but not sure when I'd actually get to it... hopefully would before the end of year...

[jimklimov]

Looking at this stalled PR and trying to remember the premise of/around the problem... remember fragments about it possibly being a problem that I added "error" returns of $backupSets (and maybe other requests) a year or so ago when I handled possibility of passing multiple arguments (array not string incoming) and as a return a numeric zero could be returned instead of the array. At least this bit felt fishy at the time too. Maybe a loud warning and returning undef or empty array, or throwing a Mojo exception, could be better responses in such cases... @oetiker : Any thoughts? :)

[oetiker]

The problem was AFAICR that this patch sort of fixes 'errors' which should not exist in the first place ... due to missing type constraints in perl we tend to end up with such problems, but I don't think it is a good thing to anticipate and fix them in the code ... rather find the origin and fix them there ...

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.