| Message ID | 20160719085432.4572-12-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > srcfifo && dstfifo must still be freed in this case. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > tests/test-io-channel-command.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c > index 1d1f461..95be370 100644 > --- a/tests/test-io-channel-command.c > +++ b/tests/test-io-channel-command.c > @@ -40,7 +40,7 @@ static void test_io_channel_command_fifo(bool async) > > unlink(TEST_FIFO); > if (access("/bin/socat", X_OK) < 0) { > - return; /* Pretend success if socat is not present */ > + goto end; /* Pretend success if socat is not present */ > } If we fail here... > if (mkfifo(TEST_FIFO, 0600) < 0) { ...then we don't create a fifo here... > abort(); > @@ -59,6 +59,7 @@ static void test_io_channel_command_fifo(bool async) > object_unref(OBJECT(src)); > object_unref(OBJECT(dst)); > > +end: > g_free(srcfifo); > g_free(dstfifo); > unlink(TEST_FIFO); ...and unlink() will (hopefully) fail to unlink a missing file. But in the worst case, it unlinks someone else's file. Probably worth being a bit stricter about only undoing what you have already done.
Hi ----- Original Message ----- > On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > srcfifo && dstfifo must still be freed in this case. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > tests/test-io-channel-command.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/tests/test-io-channel-command.c > > b/tests/test-io-channel-command.c > > index 1d1f461..95be370 100644 > > --- a/tests/test-io-channel-command.c > > +++ b/tests/test-io-channel-command.c > > @@ -40,7 +40,7 @@ static void test_io_channel_command_fifo(bool async) > > > > unlink(TEST_FIFO); > > if (access("/bin/socat", X_OK) < 0) { > > - return; /* Pretend success if socat is not present */ > > + goto end; /* Pretend success if socat is not present */ > > } > > If we fail here... > > > if (mkfifo(TEST_FIFO, 0600) < 0) { > > ...then we don't create a fifo here... > > > abort(); > > @@ -59,6 +59,7 @@ static void test_io_channel_command_fifo(bool async) > > object_unref(OBJECT(src)); > > object_unref(OBJECT(dst)); > > > > +end: > > g_free(srcfifo); > > g_free(dstfifo); > > unlink(TEST_FIFO); > > ...and unlink() will (hopefully) fail to unlink a missing file. But in > the worst case, it unlinks someone else's file. Probably worth being a > bit stricter about only undoing what you have already done. But the test starts by unlinking unconditionally too, so not sure it's really worth. > > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
Hi On Wed, Jul 20, 2016 at 1:16 AM, Marc-André Lureau <mlureau@redhat.com> wrote: > Hi > > ----- Original Message ----- >> On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote: >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> >> > >> > srcfifo && dstfifo must still be freed in this case. >> > >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> > --- >> > tests/test-io-channel-command.c | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> > diff --git a/tests/test-io-channel-command.c >> > b/tests/test-io-channel-command.c >> > index 1d1f461..95be370 100644 >> > --- a/tests/test-io-channel-command.c >> > +++ b/tests/test-io-channel-command.c >> > @@ -40,7 +40,7 @@ static void test_io_channel_command_fifo(bool async) >> > >> > unlink(TEST_FIFO); >> > if (access("/bin/socat", X_OK) < 0) { >> > - return; /* Pretend success if socat is not present */ >> > + goto end; /* Pretend success if socat is not present */ >> > } >> >> If we fail here... >> >> > if (mkfifo(TEST_FIFO, 0600) < 0) { >> >> ...then we don't create a fifo here... >> >> > abort(); >> > @@ -59,6 +59,7 @@ static void test_io_channel_command_fifo(bool async) >> > object_unref(OBJECT(src)); >> > object_unref(OBJECT(dst)); >> > >> > +end: >> > g_free(srcfifo); >> > g_free(dstfifo); >> > unlink(TEST_FIFO); >> >> ...and unlink() will (hopefully) fail to unlink a missing file. But in >> the worst case, it unlinks someone else's file. Probably worth being a >> bit stricter about only undoing what you have already done. > > But the test starts by unlinking unconditionally too, so not sure it's really worth. I fixed the test by using mkdtemp() instead, which also permits running tests concurrently.
diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c index 1d1f461..95be370 100644 --- a/tests/test-io-channel-command.c +++ b/tests/test-io-channel-command.c @@ -40,7 +40,7 @@ static void test_io_channel_command_fifo(bool async) unlink(TEST_FIFO); if (access("/bin/socat", X_OK) < 0) { - return; /* Pretend success if socat is not present */ + goto end; /* Pretend success if socat is not present */ } if (mkfifo(TEST_FIFO, 0600) < 0) { abort(); @@ -59,6 +59,7 @@ static void test_io_channel_command_fifo(bool async) object_unref(OBJECT(src)); object_unref(OBJECT(dst)); +end: g_free(srcfifo); g_free(dstfifo); unlink(TEST_FIFO);