[pulseaudio-discuss,1/8] rtp; Make init return a boolean value for success/failure

Submitted by Arun Raghavan on Feb. 29, 2016, 10:16 a.m.

Details

Message ID 1456740996-13908-2-git-send-email-arun@accosted.net
State New
Headers show
Series "Add GStreamer-based RTP support" ( rev: 1 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Arun Raghavan Feb. 29, 2016, 10:16 a.m.
From: Arun Raghavan <git@arunraghavan.net>

---
 src/modules/rtp/module-rtp-send.c | 3 ++-
 src/modules/rtp/rtp.c             | 4 ++--
 src/modules/rtp/rtp.h             | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/rtp/module-rtp-send.c b/src/modules/rtp/module-rtp-send.c
index 15a7436..22a5f44 100644
--- a/src/modules/rtp/module-rtp-send.c
+++ b/src/modules/rtp/module-rtp-send.c
@@ -489,7 +489,8 @@  int pa__init(pa_module*m) {
 
     pa_xfree(n);
 
-    pa_rtp_context_init_send(&u->rtp_context, fd, m->core->cookie, payload, pa_frame_size(&ss));
+    if (!pa_rtp_context_init_send(&u->rtp_context, fd, m->core->cookie, payload, pa_frame_size(&ss)))
+        goto fail;
     pa_sap_context_init_send(&u->sap_context, sap_fd, p);
 
     pa_log_info("RTP stream initialized with mtu %u on %s:%u from %s ttl=%u, SSRC=0x%08x, payload=%u, initial sequence #%u", mtu, dst_addr, port, src_addr, ttl, u->rtp_context.ssrc, payload, u->rtp_context.sequence);
diff --git a/src/modules/rtp/rtp.c b/src/modules/rtp/rtp.c
index 17c8d3c..f188345 100644
--- a/src/modules/rtp/rtp.c
+++ b/src/modules/rtp/rtp.c
@@ -43,7 +43,7 @@ 
 
 #include "rtp.h"
 
-pa_rtp_context* pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size) {
+bool pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size) {
     pa_assert(c);
     pa_assert(fd >= 0);
 
@@ -56,7 +56,7 @@  pa_rtp_context* pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssr
 
     pa_memchunk_reset(&c->memchunk);
 
-    return c;
+    return true;
 }
 
 #define MAX_IOVECS 16
diff --git a/src/modules/rtp/rtp.h b/src/modules/rtp/rtp.h
index bbd4278..61f23cf 100644
--- a/src/modules/rtp/rtp.h
+++ b/src/modules/rtp/rtp.h
@@ -37,7 +37,7 @@  typedef struct pa_rtp_context {
     pa_memchunk memchunk;
 } pa_rtp_context;
 
-pa_rtp_context* pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);
+bool pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);
 
 /* If the memblockq doesn't have a silence memchunk set, then the caller must
  * guarantee that the current read index doesn't point to a hole. */

Comments

On Mon, 2016-02-29 at 15:46 +0530, arun@accosted.net wrote:
> -pa_rtp_context* pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);
> +bool pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);

The function never fails, so what's the point of this patch?

(Also, a reminder: we've agreed to report function failures using a
negative int value rather than a bool.)

-- 
Tanu
On 7 April 2016 at 20:02, Tanu Kaskinen <tanuk@iki.fi> wrote:
> On Mon, 2016-02-29 at 15:46 +0530, arun@accosted.net wrote:
>> -pa_rtp_context* pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);
>> +bool pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);
>
> The function never fails, so what's the point of this patch?

I'll add this to the commit message, but this is basically for
consistency later when I add an RTP implementation which can fail.

> (Also, a reminder: we've agreed to report function failures using a
> negative int value rather than a bool.)

Was that only for external functions, or internal ones as well?

-- Arun
On Sat, 2016-04-09 at 09:36 +0530, Arun Raghavan wrote:
> On 7 April 2016 at 20:02, Tanu Kaskinen <tanuk@iki.fi> wrote:
> > 
> > On Mon, 2016-02-29 at 15:46 +0530, arun@accosted.net wrote:
> > > 
> > > -pa_rtp_context* pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);
> > > +bool pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);
> > The function never fails, so what's the point of this patch?
> I'll add this to the commit message, but this is basically for
> consistency later when I add an RTP implementation which can fail.

Ok, my original assumption was that in some later patch you'd add a
failure condition to this function, but I didn't find that. It didn't
occur to me that this could be about consistency with some other
function.

> > (Also, a reminder: we've agreed to report function failures using a
> > negative int value rather than a bool.)
> Was that only for external functions, or internal ones as well?

At least I understood the previous discussion to be about internal
functions as well. Maybe it was ambiguous and we interpreted each other
wrong (I don't have the previous discussion at hand to check).
On Sun, 2016-04-10 at 17:17 +0300, Tanu Kaskinen wrote:
> On Sat, 2016-04-09 at 09:36 +0530, Arun Raghavan wrote:
> > 
> > On 7 April 2016 at 20:02, Tanu Kaskinen <tanuk@iki.fi> wrote:
> > > 
> > > 
> > > On Mon, 2016-02-29 at 15:46 +0530, arun@accosted.net wrote:
> > > > 
> > > > 
> > > > -pa_rtp_context* pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);
> > > > +bool pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);
> > > The function never fails, so what's the point of this patch?
> > I'll add this to the commit message, but this is basically for
> > consistency later when I add an RTP implementation which can fail.
> Ok, my original assumption was that in some later patch you'd add a
> failure condition to this function, but I didn't find that. It didn't
> occur to me that this could be about consistency with some other
> function.
> 
> > 
> > > 
> > > (Also, a reminder: we've agreed to report function failures using a
> > > negative int value rather than a bool.)
> > Was that only for external functions, or internal ones as well?
> At least I understood the previous discussion to be about internal
> functions as well. Maybe it was ambiguous and we interpreted each other
> wrong (I don't have the previous discussion at hand to check).

Does it matter, then, especially since this is internal?

-- Arun
On Tue, 2016-04-12 at 10:00 +0530, Arun Raghavan wrote:
> On Sun, 2016-04-10 at 17:17 +0300, Tanu Kaskinen wrote:
> > On Sat, 2016-04-09 at 09:36 +0530, Arun Raghavan wrote:
> > > On 7 April 2016 at 20:02, Tanu Kaskinen <tanuk@iki.fi> wrote:
> > > > (Also, a reminder: we've agreed to report function failures using a
> > > > negative int value rather than a bool.)
> > > 
> > > Was that only for external functions, or internal ones as well?
> > 
> > At least I understood the previous discussion to be about internal
> > functions as well. Maybe it was ambiguous and we interpreted each other
> > wrong (I don't have the previous discussion at hand to check).

> Does it matter, then, especially since this is internal?

Well, does consistency matter in internal code? Do you want to remove
the "errors are returned from functions as negative values, success is
returned as 0 or positive value" point from
https://wiki.freedesktop.org/www/Software/PulseAudio/Documentation/Developer/CodingStyle/ ?
On 12 April 2016 at 16:33, Tanu Kaskinen <tanuk@iki.fi> wrote:
> On Tue, 2016-04-12 at 10:00 +0530, Arun Raghavan wrote:
>> On Sun, 2016-04-10 at 17:17 +0300, Tanu Kaskinen wrote:
>> > On Sat, 2016-04-09 at 09:36 +0530, Arun Raghavan wrote:
>> > > On 7 April 2016 at 20:02, Tanu Kaskinen <tanuk@iki.fi> wrote:
>> > > > (Also, a reminder: we've agreed to report function failures using a
>> > > > negative int value rather than a bool.)
>> > >
>> > > Was that only for external functions, or internal ones as well?
>> >
>> > At least I understood the previous discussion to be about internal
>> > functions as well. Maybe it was ambiguous and we interpreted each other
>> > wrong (I don't have the previous discussion at hand to check).
>>
>> Does it matter, then, especially since this is internal?
>
> Well, does consistency matter in internal code? Do you want to remove
> the "errors are returned from functions as negative values, success is
> returned as 0 or positive value" point from
> https://wiki.freedesktop.org/www/Software/PulseAudio/Documentation/Developer/CodingStyle/ ?

I do think consistency is important. otoh, in this particular case, it
introduces an artificial constraint.

IMO a negative error value only makes sense if the caller can actually
figure out the error based on the return value.

We can continue this discussion to reevaluate what makes sense from
the project perspective. I'm fine with just returning -1 in the
interest of this patch series not stalling.

-- Arun