Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cyradm perl warnings on perl v5.34.1 #4184

Open
jeffgoh opened this issue Jul 20, 2022 · 4 comments · Fixed by jeffgoh/cyrus-imapd#1 · May be fixed by #4837
Open

cyradm perl warnings on perl v5.34.1 #4184

jeffgoh opened this issue Jul 20, 2022 · 4 comments · Fixed by jeffgoh/cyrus-imapd#1 · May be fixed by #4837

Comments

@jeffgoh
Copy link

jeffgoh commented Jul 20, 2022

list mailbox with wildcard e.g. (lm user/foobar/* or lm user/foo*) will produce this error at seemingly random intervals:

Use of uninitialized value $w in numeric gt (>) at /usr/lib64/perl5/vendor_perl/Cyrus/IMAP/Shell.pm line 676.

This is on Fedora 36 - perl-interpreter-5.34.1-486.fc36.x86_64

@jeffgoh jeffgoh changed the title cyradm on cyradm perl warnings on perl v5.34.1 Jul 20, 2022
@jeffgoh
Copy link
Author

jeffgoh commented Jul 20, 2022

That was I think easily fixed:

662a663
>   $w = 0;

but I also get this, which I'm not so clear how to fix:

Negative repeat count does nothing at /usr/lib64/perl5/vendor_perl/Cyrus/IMAP/Shell.pm line 688

This is line 688:

for ($c = 0; $c < @l; $c += int((@l + $n - 1) / $n)) {

@jeffgoh
Copy link
Author

jeffgoh commented Jul 20, 2022

In the absence of comments, I couldn't tell why the code needed to be so complex - based on observed behavior, I "rewrote" the offending code segment like this:

  for (@l) {
        $lfh->[1]->print("$_","\n");
  }
  0;

diff is here:

687,693c688,690
<   for ($l = 0; $l < int((@l + $n - 1) / $n); $l++) {
<     for ($c = 0; $c < @l; $c += int((@l + $n - 1) / $n)) {
<       if ($l + $c < @l) {
<         $lfh->[1]->print($l[$l + $c], ' ' x ($w + 1 - length($l[$l + $c])));
<       }
<     }
<     $lfh->[1]->print("\n");
---
> 
>   for (@l) {
> 	$lfh->[1]->print("$_","\n");

Hopefully someone smarter than me can figure out what I might or might not have done wrong. I tested "lm user/foo*" and "lm -subscribed user/foo" when logged in as either cyrus or foobar and got the same results before/after my "patch"

@elliefm
Copy link
Contributor

elliefm commented Jul 22, 2022

I think the original code was trying to format the output into multiple columns if the terminal was wide enough and the values narrow enough.

$w looks like "the width of the widest value", $n looks like "how many columns of that width we can fit", @l looks like the list of values

And then in the double loop, the outer loop is each line of output, the inner loop is each column within that line, and all the stuffing about with $l, $c, and $n is to retrieve and print the right element from @l at the right time, with an appropriate amount of spaces following it so that the next column aligns correctly.

I can't tell at a glance whether the column display sorts downwards first or across first, some comments in here sure would be nice, wouldn't they...

I wonder if anyone ever sees the output in columns, or if real mailboxes are usually wide enough that it never happens. I guess if someone has a big monitor and runs a terminal on it full screen...

The column formatting is cute, but I'm not convinced it's necessary. It's also hard to read -- at a glance I don't see a likely cause of the "Negative repeat count" warning, nor is the display order obvious -- and hard to read is the same as hard to fix.

Your patch looks like it solves the problems in the complex column processing by just always outputting a single column instead, am I right? That seems reasonable to me. Do you want to submit this as a PR?

I think all the calculation of $w, $ll, $n, $c are unnecessary once the column formatting is discarded, so you should also remove the parts that calculate those values. $l and @l look like they're still needed though.

jeffgoh added a commit to jeffgoh/cyrus-imapd that referenced this issue Mar 9, 2024
Simplify multi-column to single column fixes cyrusimap#4184
@jeffgoh jeffgoh reopened this Mar 9, 2024
@jeffgoh jeffgoh linked a pull request Mar 9, 2024 that will close this issue
@mosvald
Copy link
Contributor

mosvald commented Jul 11, 2024

I could reproduce this easily with:

localhost> cm user.12345678901234567890123456789012345678901234567890123456789
localhost> lm
Negative repeat count does nothing at /usr/lib64/perl5/vendor_perl/Cyrus/IMAP/Shell.pm line 689.
user.12345678901234567890123456789012345678901234567890123456789 (\HasNoChildren)
localhost>

interestingly, if you create the mailbox one byte shorter, it doesn't print this msg:

localhost> cm user.1234567890123456789012345678901234567890123456789012345678 
localhost> lm       
user.1234567890123456789012345678901234567890123456789012345678 (\HasNoChildren)
localhost>

The Use of uninitialized value $w was fixed by 42a10f4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants