Re: [RFC] channels regression issue

[Heiko Thiery <heiko.thiery@xxxxxxxxx>]

Hi Jakub,

Am Di., 21. Apr. 2020 um 10:59 Uhr schrieb Jakub Jelen <jjelen@xxxxxxxxxx>:
>
> On Thu, 2020-04-16 at 16:05 +0200, Heiko Thiery wrote:
> > Hi Jakub and all,
> >
> > > Tests are living in the tests/ directory. This does not sound like
> > > something that could be covered with unit test so either client or
> > > server directory would be appropriate. We do not have much code
> > > coverage for this low-level channel handling so it is appropriate
> > > to
> > > create a new test based either of existing ones invoking this
> > > timeout
> > > and this bug. The testsuite already allows you to start openssh
> > > servers
> > > (most of the tests under client/) or invoke existing clients (for
> > > example pkd/ tests).
> > >
> > > Let me know if you will have some trouble putting things together.
> >
> > --- a/tests/client/torture_session.c
> > +++ b/tests/client/torture_session.c
> > @@ -118,12 +118,45 @@ static void torture_channel_read_error(void
> > **state) {
> >      ssh_channel_free(channel);
> >  }
> >
> > +static void torture_channel_poll_timeout(void **state) {
> > +    struct torture_state *s = *state;
> > +    ssh_session session = s->ssh.session;
> > +    ssh_channel channel;
> > +    int rc;
> > +    int fd;
> > +    int i;
> > +
> > +    channel = ssh_channel_new(session);
> > +    assert_non_null(channel);
> > +
> > +    rc = ssh_channel_open_session(channel);
> > +    assert_ssh_return_code(session, rc);
> > +
> > +    fd = ssh_get_fd(session);
> > +    assert_true(fd > 2);
> > +
> > +       rc = ssh_channel_poll_timeout(channel, 500, 0);
> > +       assert_int_equal(rc, SSH_OK);
> > +
> > +    /* send crap and for server to send us a disconnect */
> > +    rc = write(fd, "AAAA", 4);
> > +    assert_int_equal(rc, 4);
> > +
> > +       rc = ssh_channel_poll_timeout(channel, 500, 0);
> > +       assert_int_equal(rc, SSH_ERROR);
> > +
> > +    ssh_channel_free(channel);
> > +}
> > +
> >  int torture_run_tests(void) {
> >      int rc;
> >      struct CMUnitTest tests[] = {
> >          cmocka_unit_test_setup_teardown(torture_channel_read_error,
> >                                          session_setup,
> >                                          session_teardown),
> > +        cmocka_unit_test_setup_teardown(torture_channel_poll_timeout
> > ,
> > +                                        session_setup,
> > +                                        session_teardown),
> >      };
> >
> >      ssh_init();
> >
> > is this somehow make sense here? frankly speaking I don't excactly
> > know if this is the right position to do this test. A good case that
> > returns valid data is missing.
> >
> > What do you think?
>
> I tested this with and without my patch in master and it looks like it
> reproduces the issue just fine. The only nit is that the variable i is
> unused in your patch and there is some indentation mismatch.
>
> Please, submit it as a merge request on the gitlab so it will not get
> lost here.

No problem. Do you have an idea what to do for checking the valid case
with returned data?

> Thanks,
> Jakub
>
>