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

format-table: make all widths be set properly #9550

Merged
merged 3 commits into from Jul 16, 2018

Conversation

2 participants
@nosada
Contributor

nosada commented Jul 9, 2018

This commit fixes #9549.

  1. Remove assert() in table_print():

    assert(!restart);

  2. Check finalize when breaking for loop in table_print():

    if (restart)

@yuwata

When both finalize == true and restart == true, then some width may not be assigned... I think we should not break when finalize == true. That is,

if (restart)
        break;

should be

if (restart && !finalize)
        break;
@@ -1139,7 +1139,6 @@ int table_print(Table *t, FILE *f) {
}

if (finalize) {
assert(!restart);
break;
}

This comment has been minimized.

@yuwata

yuwata Jul 9, 2018

Member

Please remove unnecessary {}.

This comment has been minimized.

@nosada

nosada Jul 10, 2018

Contributor

Ah, sorry I forgot it.
But thanks to your comment I noticed this deletion seems not to make any sense, because checking finalize has done before reaching L:1142 after changing break condition.

I'll revert it.

This comment has been minimized.

@nosada

nosada Jul 10, 2018

Contributor

My confusion... Deleting assert() is needed to fix #9549.

@yuwata

This comment has been minimized.

Member

yuwata commented Jul 10, 2018

The issue seems not restricted to machinectl. So, please update the commit message when you update this.

@nosada nosada changed the title from machinectl: remove assert() in table_print() to remove assert() in table_print() Jul 10, 2018

@nosada nosada changed the title from remove assert() in table_print() to also check 'finalize' when breaking for loop in table_print() Jul 10, 2018

@nosada

This comment has been minimized.

Contributor

nosada commented Jul 10, 2018

Updated by getting into line with @yuwata's comment.

I also updated the title and description of this PR because original doesn't make sense now, but I wonder this is correct for the PR policy of yours.
Should I recreate another PR? or can I keep this up?

@nosada nosada changed the title from also check 'finalize' when breaking for loop in table_print() to also check 'finalize' when breaking for loop and delete assert() in table_print() Jul 10, 2018

@yuwata

This comment has been minimized.

Member

yuwata commented Jul 10, 2018

LGTM, but I have not tested well.

Should I recreate another PR? or can I keep this up?

In this case, the change is sufficiently small, so we will squash the commits on merge.

@nosada

This comment has been minimized.

Contributor

nosada commented Jul 11, 2018

Thanks, I understood.
And sorry for being late to tell you that machinectl was already removed from commit message.

@yuwata

This comment has been minimized.

Member

yuwata commented Jul 11, 2018

BTW, it is not necessary, but if possible, could you provide any reproducers in src/test/test-format-table.c? Then, we can easily judge what is the problem and actually this fixes the issue.

@nosada

This comment has been minimized.

Contributor

nosada commented Jul 11, 2018

Reported issue in #9549 can be reproduced like below.
At least I checked my PR in below procedure whether the issue is fixed or not.

  1. Make at least 3 images visible in machinectl list-images
  2. Do machinectl list-images | less or machinectl list-images > foo
  3. If problem is fixed, commands invoked in 2. will success. Get assertion if otherwise

Examples in failure:

$ machinectl list-images
NAME  TYPE RO  USAGE CREATED                     MODIFIED
foo   raw  no   1.0G Wed 2018-07-11 00:10:33 JST Wed 2018-07-11 00:16:00 JST
bar   raw  no 673.7M Wed 2018-07-11 00:23:06 JST Wed 2018-07-11 00:28:06 JST
baz   raw  no   2.1G Tue 2018-07-10 23:54:08 JST Wed 2018-07-11 00:04:18 JST
qux   raw  no 687.4M Tue 2018-07-10 23:45:46 JST Tue 2018-07-10 23:50:28 JST

4 images listed.

$ machinectl list-images | less
(got `Assertion '!restart' failed at ../systemd-stable/src/basic/format-table.c:1142, function table_print(). Aborting.`)

$ machinectl list-images > hoge
Assertion '!restart' failed at ../systemd-stable/src/basic/format-table.c:1142, function table_print(). Aborting.
Aborted (core dumped)

$ cat hoge

$

Examples in success:

$ machinectl list-images
NAME  TYPE RO  USAGE CREATED                     MODIFIED
foo   raw  no   1.0G Wed 2018-07-11 00:10:33 JST Wed 2018-07-11 00:16:00 JST
bar   raw  no 673.7M Wed 2018-07-11 00:23:06 JST Wed 2018-07-11 00:28:06 JST
baz   raw  no   2.1G Tue 2018-07-10 23:54:08 JST Wed 2018-07-11 00:04:18 JST
qux   raw  no 687.4M Tue 2018-07-10 23:45:46 JST Tue 2018-07-10 23:50:28 JST

$ machinectl list-images | less
(got same output to `machinectl list-images`)

$ machinectl list-images > hoge

$ cat hoge
(got same output too)

$

I hope this comment will help you.

@yuwata

This comment has been minimized.

Member

yuwata commented Jul 12, 2018

I've squashed the commits by @nosada and added a reproducer in test-format-table.c.
The test fails without the first commit.
PTAL.

@yuwata yuwata changed the title from also check 'finalize' when breaking for loop and delete assert() in table_print() to format-table: make all widths be set properly Jul 12, 2018

@nosada

This comment has been minimized.

Contributor

nosada commented Jul 12, 2018

Sorry... I misread.
You exactly wrote about 'reproducers' in src/test/test-format-table.c, not sequence of commands to reproduce problem (I'm ashamed to write I thought this is called as 'reproducer').
And I didn't use any reproducers to create this PR... I checked my fixing by typing commands written in #9550 (comment) manually.

Reproducer b0804bb seems to work fine because:

  • this fails when a26db0b not cherry-pick
  • after cherry-pick a26db0b this successed
@nosada

This comment has been minimized.

Contributor

nosada commented Jul 16, 2018

@yuwata
I would want to ask you that there are any other points in this PR to fix for #9550 (review).

This review seems to be answered by a26db0b (squashed by you) already and contains any other of what is needed to change for it.

@yuwata

This comment has been minimized.

Member

yuwata commented Jul 16, 2018

CI failures are not related. Let's merge this.

@yuwata yuwata merged commit 460d7ac into systemd:master Jul 16, 2018

5 of 7 checks passed

Fedora Rawhide CI x86_64 rpm build [internal error]
Details
LGTM analysis: C/C++ Analysis Failed (could not build the merge commit)
Details
LGTM analysis: JavaScript No alert changes
Details
bionic-amd64 autopkgtest finished (success)
Details
bionic-i386 autopkgtest finished (success)
Details
bionic-s390x autopkgtest finished (success)
Details
semaphoreci The build passed on Semaphore.
Details
@nosada

This comment has been minimized.

Contributor

nosada commented Jul 16, 2018

Oh, I see, that was claimed by CI... Thank you responding and merging.

@nosada nosada deleted the nosada:fix-9549 branch Jul 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment