In D43475#991182, @bofh wrote:

In D43475#991153, @markj wrote:

I don't see what's preventing you from pushing the kyua vendor branch to a github fork. And that's not necessary anyway, if you just push the merge commit somewhere, that's sufficient.

Here is the branch https://github.com/5u623l20/freebsd-src/tree/kyua-merge

In D43475#991153, @markj wrote:

I don't see what's preventing you from pushing the kyua vendor branch to a github fork. And that's not necessary anyway, if you just push the merge commit somewhere, that's sufficient.

Here is the branch https://github.com/5u623l20/freebsd-src/tree/kyua-merge

In D43475#991166, @bofh wrote:

In D43475#991153, @markj wrote:

Looks sane, thanks. I ran through the test suite locally with this change and didn't spot any new problems.

My tests did not yield any errors.

Let us know if you'd like review of the merge commit itself, i.e., push the merge somewhere that we can inspect it.

It's a bit difficult pushing a merge commit somewhere else specially Github. As the vendor trees are actually not present in a Github fork. If you want I can create custom branch and apply the patch manually and push it to a new branch if it helps.

In D43475#991153, @markj wrote:

Looks sane, thanks. I ran through the test suite locally with this change and didn't spot any new problems.

My tests did not yield any errors.

Looks sane, thanks. I ran through the test suite locally with this change and didn't spot any new problems.

I've been able to trigger an sbwait() assertion failure in the receive path when running KTLS tests, but only when running tests in parallel in QEMU. I believe the problem is the following: KTLS introduces a state for received data wherein it is being decrypted but is not yet queued in the socket buffer. In this case (sb_tlsdcc != 0), we can end up calling sbwait() with CANTRECVMORE set because that sleep/wakeup mechanism is used to signal completion of decryption operations.

Thanks, Mark, I will send you two commits as requested. Thanks again for all your help!

Looks good to me. Thanks for working on this.

Reformat to ahere to suggested style...

@ngie @lwhsu Hello, I found this on https://wiki.freebsd.org/JuniorJobs, Please close this. This is more involved than what i could comprehend from that page.

In D32736#743547, @mckusick wrote:

Sorry for my long delay.

I concur that it is my change that should be fixed, not these tests.

I believe that the correct fix is this:

diff --git a/sbin/geom/core/geom.c b/sbin/geom/core/geom.c
index 2e0d8683df49..0202be9a063e 100644
--- a/sbin/geom/core/geom.c
+++ b/sbin/geom/core/geom.c
@@ -445,7 +445,7 @@ set_flags(struct g_command *cmd)
 {
        unsigned flags = 0;

-       if ((cmd->gc_flags & G_FLAG_VERBOSE) != 0)
+       if ((cmd->gc_flags & G_FLAG_VERBOSE) != 0 && verbose)
                flags |= G_FLAG_VERBOSE;

        return (flags);

Sorry for my long delay.

In D32736#741235, @olivier wrote:

Shouldn't the root cause be fixed (geom be revert back to non-verbose mode by default) in place of fixing the regression test ?

Shouldn't the root cause be fixed (geom be revert back to non-verbose mode by default) in place of fixing the regression test ?

I am also checking this, but what I haven't figured out is that it seems these test cases don't execute geom utility commands with -v so it should not expect the output from verbose mode. Is there anything I missed that those test cases do enable the verbose flag?

Do these utilities always run in verbose mode now?

In D23193#627863, @fernando.valle_eldorado.org.br wrote:

This review was approved in March last year. It seems all issues have been solved. Could anyone commit this?

This review was approved in March last year. It seems all issues have been solved. Could anyone commit this?

This change does more than advertised in the CR description (which doesn't describe as much as the commit message should).

@jmg ping, please review..

In D26220#588628, @pnagato_protonmail.com wrote:

In D26220#588409, @pnagato_protonmail.com wrote:

@imp if the test binary is run directly like this. We can see the assertion output.

./sbuf_core_test sbuf_new_negative_test_non_user_flags

sbuf_core_test: WARNING: Running test cases outside of kyua(1) is unsupported
sbuf_core_test: WARNING: No isolation nor timeout control is being applied; you may get unexpected failures; see atf-test-case(4)
expected_death: Non user flags cannot be specified
Assertion failed: (((flags & ~0x0000ffff) == 0)), function sbuf_new, file /usr/src/sys/kern/subr_sbuf.c, line 233.
Abort (core dumped)

@imp tried the do while.. & assert. In both cases kyua does not print to log file. I see the msg only when the binary is run standalone or a single test case is run. For e.g.

kyua debug sbuf_core_test:sbuf_new_negative_test
Assertion failed: ((length >= 0)), function sbuf_new, file /usr/src/sys/kern/subr_sbuf.c, line 231.
Process with PID 29505 exited with signal 6 and dumped core; attempting to gather stack trace
Cannot find GDB binary; builtin was 'gdb'
sbuf_core_test:sbuf_new_negative_test  ->  expected_failure: Buffer length cannot be negative

@imp I removed the output from the kyua log from this comment.

In D26220#588409, @pnagato_protonmail.com wrote:

@imp if the test binary is run directly like this. We can see the assertion output.

./sbuf_core_test sbuf_new_negative_test_non_user_flags

sbuf_core_test: WARNING: Running test cases outside of kyua(1) is unsupported
sbuf_core_test: WARNING: No isolation nor timeout control is being applied; you may get unexpected failures; see atf-test-case(4)
expected_death: Non user flags cannot be specified
Assertion failed: (((flags & ~0x0000ffff) == 0)), function sbuf_new, file /usr/src/sys/kern/subr_sbuf.c, line 233.
Abort (core dumped)

@imp if the test binary is run directly like this. We can see the assertion output.

atf_tc_expect_death("....") will print the reason. For e.g.

In D26220#588231, @jmg wrote:

In D26220#587460, @pnagato_protonmail.com wrote:

@jmg @imp I have seen other places where KASSERT was re-defined as

#define	KASSERT(exp,msg) assert((exp))

I think using assert should be used. It doesn't pollute stdout, generates a core dump, and is more obvious that there was a failure.

Thanks.

@jmg Please review.

Add negative tests for sbuf_new

In D26220#587460, @pnagato_protonmail.com wrote:

@jmg @imp I have seen other places where KASSERT was re-defined as

#define	KASSERT(exp,msg) assert((exp))

In D26220#584735, @pnagato_protonmail.com wrote:

In D26220#584584, @jmg wrote:

In D26220#584528, @pnagato_protonmail.com wrote:

In D26220#584521, @imp wrote:

In D26220#584195, @pnagato_protonmail.com wrote:

@jmg @imp Need your input how to proceed on the sbuf_negative test.

I'm having trouble understanding its purpose. Maybe you could give a one or two sentence summary of what it should test?

@imp I am assuming the sbuf_new_negative_test is supposed to test for cases for which sbuf_new fails to create a sbuf.

From sys/kern/subr_sbuf.c

	KASSERT(length >= 0,
	    ("attempt to create an sbuf of negative length (%d)", length));
	KASSERT((flags & ~SBUF_USRFLAGMSK) == 0,
	    ("%s called with invalid flags", __func__));

or when SBMALLOC fails..

So, looking at the KASSERT, we may want to change how KASSERTs are compiled for userland. Right now it gets compiled to nothing, which means that there is nothing preventing userland from passing negative lengths and the like. We may want to think about making KASSERT call abort, but then some KASSERTs (like those in sbuf_new quoted above) need to be changed because aborting when length, which might be a user defined parameter seems like a bad idea.

Also, it looks like sbuf.9 doesn't document the KASSERT restrictions in sbuf_new, so a userland caller may accidentally generate a corrupted sbuf.

As for how to make sbuf fail due to malloc, I don't know.

@jmg i will skip the sbuf_new_negative_test for now. Please review the other tests.

In D26220#584584, @jmg wrote:

In D26220#584528, @pnagato_protonmail.com wrote:

In D26220#584521, @imp wrote:

In D26220#584195, @pnagato_protonmail.com wrote:

@jmg @imp Need your input how to proceed on the sbuf_negative test.

I'm having trouble understanding its purpose. Maybe you could give a one or two sentence summary of what it should test?

@imp I am assuming the sbuf_new_negative_test is supposed to test for cases for which sbuf_new fails to create a sbuf.

From sys/kern/subr_sbuf.c

	KASSERT(length >= 0,
	    ("attempt to create an sbuf of negative length (%d)", length));
	KASSERT((flags & ~SBUF_USRFLAGMSK) == 0,
	    ("%s called with invalid flags", __func__));

or when SBMALLOC fails..

So, looking at the KASSERT, we may want to change how KASSERTs are compiled for userland. Right now it gets compiled to nothing, which means that there is nothing preventing userland from passing negative lengths and the like. We may want to think about making KASSERT call abort, but then some KASSERTs (like those in sbuf_new quoted above) need to be changed because aborting when length, which might be a user defined parameter seems like a bad idea.

Also, it looks like sbuf.9 doesn't document the KASSERT restrictions in sbuf_new, so a userland caller may accidentally generate a corrupted sbuf.

As for how to make sbuf fail due to malloc, I don't know.

Skip sbuf_new_negative_test,implement sbuf_new_positive_test

In D26220#584528, @pnagato_protonmail.com wrote:

In D26220#584521, @imp wrote:

In D26220#584195, @pnagato_protonmail.com wrote:

@jmg @imp Need your input how to proceed on the sbuf_negative test.

I'm having trouble understanding its purpose. Maybe you could give a one or two sentence summary of what it should test?

@imp I am assuming the sbuf_new_negative_test is supposed to test for cases for which sbuf_new fails to create a sbuf.

From sys/kern/subr_sbuf.c

	KASSERT(length >= 0,
	    ("attempt to create an sbuf of negative length (%d)", length));
	KASSERT((flags & ~SBUF_USRFLAGMSK) == 0,
	    ("%s called with invalid flags", __func__));

or when SBMALLOC fails..