All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] seq: arecordmidi: Add num-events option
@ 2019-01-29  1:44 pmalani
  2019-01-29  3:36 ` Takashi Sakamoto
  0 siblings, 1 reply; 11+ messages in thread
From: pmalani @ 2019-01-29  1:44 UTC (permalink / raw)
  To: patch; +Cc: Prashant Malani, alsa-devel, derat

From: Prashant Malani <pmalani@chromium.org>

Add a command line option to automatically exit after recording a fixed
number of MIDI events. This allows a program using arecordmidi to expect
a MIDI file to be written automatically when the specified number of
events have been received, instead of having to send a SIGINT or SIGTERM
programmatically.

It also avoids the need to have the arecordmidi process running in the
background, and then constantly stat the output file to check if any
bytes have been written to it (this makes for less predictable and
longer-running tests).

This functionality finds use in Chrome OS functional testing, since
having to send SIGTERM/SIGINT programmatically and then wait for the
output file adds unpredictability and delay to the tests.

The addition of this command-line option should (hopefully) not break
any existing usage.

Signed-off-by: Prashant Malani <pmalani@chromium.org>

diff --git a/seq/aplaymidi/arecordmidi.c b/seq/aplaymidi/arecordmidi.c
index 19dbb7d..ca25bfa 100644
--- a/seq/aplaymidi/arecordmidi.c
+++ b/seq/aplaymidi/arecordmidi.c
@@ -690,7 +690,8 @@ static void help(const char *argv0)
 		"  -t,--ticks=ticks           resolution in ticks per beat or frame\n"
 		"  -s,--split-channels        create a track for each channel\n"
 		"  -m,--metronome=client:port play a metronome signal\n"
-		"  -i,--timesig=nn:dd         time signature\n",
+		"  -i,--timesig=nn:dd         time signature\n"
+		"  -n,--num-events=events     fixed number of events to record, then exit\n",
 		argv0);
 }
 
@@ -706,7 +707,7 @@ static void sighandler(int sig)
 
 int main(int argc, char *argv[])
 {
-	static const char short_options[] = "hVlp:b:f:t:sdm:i:";
+	static const char short_options[] = "hVlp:b:f:t:sdm:i:n:";
 	static const struct option long_options[] = {
 		{"help", 0, NULL, 'h'},
 		{"version", 0, NULL, 'V'},
@@ -719,6 +720,7 @@ int main(int argc, char *argv[])
 		{"dump", 0, NULL, 'd'},
 		{"metronome", 1, NULL, 'm'},
 		{"timesig", 1, NULL, 'i'},
+		{"num-events", 1, NULL, 'n'},
 		{ }
 	};
 
@@ -727,6 +729,9 @@ int main(int argc, char *argv[])
 	struct pollfd *pfds;
 	int npfds;
 	int c, err;
+	/* If |num_events| isn't specified, leave it at 0. */
+	int num_events = 0;
+	int events_received = 0;
 
 	init_seq();
 
@@ -775,6 +780,11 @@ int main(int argc, char *argv[])
 		case 'i':
 			time_signature(optarg);
 			break;
+		case 'n':
+			num_events = atoi(optarg);
+			if (num_events <= 0)
+				fatal("num_events must be greater than 0");
+			break;
 		default:
 			help(argv[0]);
 			return 1;
@@ -864,13 +874,20 @@ int main(int argc, char *argv[])
 			err = snd_seq_event_input(seq, &event);
 			if (err < 0)
 				break;
-			if (event)
+			if (event) {
 				record_event(event);
+				events_received++;
+			}
 		} while (err > 0);
 		if (stop)
 			break;
+		if (num_events && (events_received == num_events))
+			break;
 	}
 
+	if (num_events && events_received < num_events)
+		fputs("Warning: Received signal before num_events\n", stdout);
+
 	finish_tracks();
 	write_file();
 
-- 
2.20.1.495.gaa96b0ce6b-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] seq: arecordmidi: Add num-events option
  2019-01-29  1:44 [PATCH 1/1] seq: arecordmidi: Add num-events option pmalani
@ 2019-01-29  3:36 ` Takashi Sakamoto
  2019-01-29  5:34   ` Prashant Malani
  0 siblings, 1 reply; 11+ messages in thread
From: Takashi Sakamoto @ 2019-01-29  3:36 UTC (permalink / raw)
  To: pmalani, patch; +Cc: alsa-devel, derat

Hi,

On 2019/01/29 10:44, pmalani@chromium.org wrote:
> @@ -775,6 +780,11 @@ int main(int argc, char *argv[])
> +		case 'n':
> +			num_events = atoi(optarg);
> +			if (num_events <= 0)
> +				fatal("num_events must be greater than 0");
> +			break;

We cannot distinguish two cases; 0 is given and including non-numeraical
characters. Please use strtol() or so and check return value and errno
for safe.

For your information:
http://git.alsa-project.org/?p=alsa-utils.git;a=blob;f=axfer/main.c;hb=HEAD#l44


Regards

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] seq: arecordmidi: Add num-events option
  2019-01-29  3:36 ` Takashi Sakamoto
@ 2019-01-29  5:34   ` Prashant Malani
  2019-01-29  8:45     ` Takashi Sakamoto
  0 siblings, 1 reply; 11+ messages in thread
From: Prashant Malani @ 2019-01-29  5:34 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, patch, Daniel Erat

Hi Takashi,

On Mon, Jan 28, 2019 at 7:36 PM Takashi Sakamoto <o-takashi@sakamocchi.jp>
wrote:

> Hi,
>
> On 2019/01/29 10:44, pmalani@chromium.org wrote:
> > @@ -775,6 +780,11 @@ int main(int argc, char *argv[])
> > +             case 'n':
> > +                     num_events = atoi(optarg);
> > +                     if (num_events <= 0)
> > +                             fatal("num_events must be greater than 0");
> > +                     break;
>
> We cannot distinguish two cases; 0 is given and including non-numeraical
> characters. Please use strtol() or so and check return value and errno
> for safe.
>
> For your information:
>
> http://git.alsa-project.org/?p=alsa-utils.git;a=blob;f=axfer/main.c;hb=HEAD#l44

Can I duplicate this in arecordmidi.c ? I'm loath to introduce a header,
and then refactor axfer/main.c.

>
>
>
> Regards
>
> Takashi Sakamoto
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] seq: arecordmidi: Add num-events option
  2019-01-29  5:34   ` Prashant Malani
@ 2019-01-29  8:45     ` Takashi Sakamoto
  2019-01-29  9:58       ` pmalani
  2019-01-29 16:18       ` Clemens Ladisch
  0 siblings, 2 replies; 11+ messages in thread
From: Takashi Sakamoto @ 2019-01-29  8:45 UTC (permalink / raw)
  To: Prashant Malani; +Cc: alsa-devel

Hi,

On Mon, Jan 28, 2019 at 09:34:59PM -0800, Prashant Malani wrote:
> Can I duplicate this in arecordmidi.c ? I'm loath to introduce a
> header, and then refactor axfer/main.c.

I don't mind it.

When parsing string with non-numeric characters, the behaviour of
atoi() is undefined still in C11.

$ cd alsa-utils.git
$ git grep atoi | grep optarg | wc
     39     158    1919

Mmm... Many parts are written without enough care of the undefined
behaviour...


Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/1] seq: arecordmidi: Add num-events option
  2019-01-29  8:45     ` Takashi Sakamoto
@ 2019-01-29  9:58       ` pmalani
  2019-01-29 16:18       ` Clemens Ladisch
  1 sibling, 0 replies; 11+ messages in thread
From: pmalani @ 2019-01-29  9:58 UTC (permalink / raw)
  To: patch; +Cc: Prashant Malani, alsa-devel, derat

From: Prashant Malani <pmalani@chromium.org>

Add a command line option to automatically exit after recording a fixed
number of MIDI events. This allows a program using arecordmidi to expect
a MIDI file to be written automatically when the specified number of
events have been received, instead of having to send a SIGINT or SIGTERM
programmatically.

It also avoids the need to have the arecordmidi process running in the
background, and then constantly stat the output file to check if any
bytes have been written to it (this makes for less predictable and
longer-running tests).

This functionality finds use in Chrome OS functional testing, since
having to send SIGTERM/SIGINT programmatically and then wait for the
output file adds unpredictability and delay to the tests.

The addition of this command-line option should (hopefully) not break
any existing usage.

Signed-off-by: Prashant Malani <pmalani@chromium.org>

diff --git a/seq/aplaymidi/arecordmidi.c b/seq/aplaymidi/arecordmidi.c
index 19dbb7d..c0d0569 100644
--- a/seq/aplaymidi/arecordmidi.c
+++ b/seq/aplaymidi/arecordmidi.c
@@ -86,6 +86,25 @@ static int ts_num = 4; /* time signature: numerator */
 static int ts_div = 4; /* time signature: denominator */
 static int ts_dd = 2; /* time signature: denominator as a power of two */
 
+/* Parse a decimal number from a command line argument. */
+static long arg_parse_decimal_num(const char *str, int *err)
+{
+	long val;
+	char *endptr;
+
+	errno = 0;
+	val = strtol(str, &endptr, 0);
+	if (errno > 0) {
+		*err = -errno;
+		return 0;
+	}
+	if (*endptr != '\0') {
+		*err = -EINVAL;
+		return 0;
+	}
+
+	return val;
+}
 
 /* prints an error message to stderr, and dies */
 static void fatal(const char *msg, ...)
@@ -690,7 +709,8 @@ static void help(const char *argv0)
 		"  -t,--ticks=ticks           resolution in ticks per beat or frame\n"
 		"  -s,--split-channels        create a track for each channel\n"
 		"  -m,--metronome=client:port play a metronome signal\n"
-		"  -i,--timesig=nn:dd         time signature\n",
+		"  -i,--timesig=nn:dd         time signature\n"
+		"  -n,--num-events=events     fixed number of events to record, then exit\n",
 		argv0);
 }
 
@@ -706,7 +726,7 @@ static void sighandler(int sig)
 
 int main(int argc, char *argv[])
 {
-	static const char short_options[] = "hVlp:b:f:t:sdm:i:";
+	static const char short_options[] = "hVlp:b:f:t:sdm:i:n:";
 	static const struct option long_options[] = {
 		{"help", 0, NULL, 'h'},
 		{"version", 0, NULL, 'V'},
@@ -719,6 +739,7 @@ int main(int argc, char *argv[])
 		{"dump", 0, NULL, 'd'},
 		{"metronome", 1, NULL, 'm'},
 		{"timesig", 1, NULL, 'i'},
+		{"num-events", 1, NULL, 'n'},
 		{ }
 	};
 
@@ -727,6 +748,9 @@ int main(int argc, char *argv[])
 	struct pollfd *pfds;
 	int npfds;
 	int c, err;
+	/* If |num_events| isn't specified, leave it at 0. */
+	long num_events = 0;
+	long events_received = 0;
 
 	init_seq();
 
@@ -775,6 +799,16 @@ int main(int argc, char *argv[])
 		case 'i':
 			time_signature(optarg);
 			break;
+		case 'n':
+			err = 0;
+			num_events = arg_parse_decimal_num(optarg, &err);
+			if (err != 0) {
+				fatal("Couldn't parse num_events argument: %s\n",
+					strerror(-err));
+			}
+			if (num_events <= 0)
+				fatal("num_events must be greater than 0");
+			break;
 		default:
 			help(argv[0]);
 			return 1;
@@ -864,13 +898,20 @@ int main(int argc, char *argv[])
 			err = snd_seq_event_input(seq, &event);
 			if (err < 0)
 				break;
-			if (event)
+			if (event) {
 				record_event(event);
+				events_received++;
+			}
 		} while (err > 0);
 		if (stop)
 			break;
+		if (num_events && (events_received == num_events))
+			break;
 	}
 
+	if (num_events && events_received < num_events)
+		fputs("Warning: Received signal before num_events\n", stdout);
+
 	finish_tracks();
 	write_file();
 
-- 
2.20.1.495.gaa96b0ce6b-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] seq: arecordmidi: Add num-events option
  2019-01-29  8:45     ` Takashi Sakamoto
  2019-01-29  9:58       ` pmalani
@ 2019-01-29 16:18       ` Clemens Ladisch
  2019-01-29 23:42         ` Prashant Malani
  1 sibling, 1 reply; 11+ messages in thread
From: Clemens Ladisch @ 2019-01-29 16:18 UTC (permalink / raw)
  To: alsa-devel, Takashi Sakamoto; +Cc: Prashant Malani

Takashi Sakamoto wrote:
> When parsing string with non-numeric characters, the behaviour of
> atoi() is undefined still in C11.

A completely non-numeric string is specified to return zero.

The behaviour is undefined only if the value cannot be represented, i.e.,
if it overflows.


Regards,
Clemens

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] seq: arecordmidi: Add num-events option
  2019-01-29 16:18       ` Clemens Ladisch
@ 2019-01-29 23:42         ` Prashant Malani
  2019-02-04 20:20           ` Prashant Malani
  0 siblings, 1 reply; 11+ messages in thread
From: Prashant Malani @ 2019-01-29 23:42 UTC (permalink / raw)
  To: Clemens Ladisch; +Cc: alsa-devel

Interesting. Thanks for pointing that out, Clemens.

In any case, I've sent both versions (atoi v/s strtol), so whichever one is
preferred can be used (would be slightly inclined towards atoi, since it's
used elsewhere in the file, and because of Clemens' comment; additionally,
there is less code duplication with the atoi version).

Best regards,

On Tue, Jan 29, 2019 at 8:19 AM Clemens Ladisch <clemens@ladisch.de> wrote:

> Takashi Sakamoto wrote:
> > When parsing string with non-numeric characters, the behaviour of
> > atoi() is undefined still in C11.
>
> A completely non-numeric string is specified to return zero.
>
> The behaviour is undefined only if the value cannot be represented, i.e.,
> if it overflows.
>
>
> Regards,
> Clemens
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] seq: arecordmidi: Add num-events option
  2019-01-29 23:42         ` Prashant Malani
@ 2019-02-04 20:20           ` Prashant Malani
  2019-02-11  4:23             ` Prashant Malani
  0 siblings, 1 reply; 11+ messages in thread
From: Prashant Malani @ 2019-02-04 20:20 UTC (permalink / raw)
  To: Clemens Ladisch; +Cc: alsa-devel

Friendly ping. Sounds like Clemens mentioned atoi() should be OK (given his
explanation regarding non-numeric strings, along with consistency with the
rest of the utility).
Are there any other concerns with this patchset?

Thanks once again for taking the time to review.

Best regards,

On Tue, Jan 29, 2019 at 3:42 PM Prashant Malani <pmalani@chromium.org>
wrote:

> Interesting. Thanks for pointing that out, Clemens.
>
> In any case, I've sent both versions (atoi v/s strtol), so whichever one
> is preferred can be used (would be slightly inclined towards atoi, since
> it's used elsewhere in the file, and because of Clemens' comment;
> additionally, there is less code duplication with the atoi version).
>
> Best regards,
>
> On Tue, Jan 29, 2019 at 8:19 AM Clemens Ladisch <clemens@ladisch.de>
> wrote:
>
>> Takashi Sakamoto wrote:
>> > When parsing string with non-numeric characters, the behaviour of
>> > atoi() is undefined still in C11.
>>
>> A completely non-numeric string is specified to return zero.
>>
>> The behaviour is undefined only if the value cannot be represented, i.e.,
>> if it overflows.
>>
>>
>> Regards,
>> Clemens
>>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] seq: arecordmidi: Add num-events option
  2019-02-04 20:20           ` Prashant Malani
@ 2019-02-11  4:23             ` Prashant Malani
  2019-02-11  8:14               ` Takashi Iwai
  0 siblings, 1 reply; 11+ messages in thread
From: Prashant Malani @ 2019-02-11  4:23 UTC (permalink / raw)
  To: Clemens Ladisch; +Cc: alsa-devel

Friendly ping.

On Mon, Feb 4, 2019 at 12:20 PM Prashant Malani <pmalani@chromium.org>
wrote:

> Friendly ping. Sounds like Clemens mentioned atoi() should be OK (given
> his explanation regarding non-numeric strings, along with consistency with
> the rest of the utility).
> Are there any other concerns with this patchset?
>
> Thanks once again for taking the time to review.
>
> Best regards,
>
> On Tue, Jan 29, 2019 at 3:42 PM Prashant Malani <pmalani@chromium.org>
> wrote:
>
>> Interesting. Thanks for pointing that out, Clemens.
>>
>> In any case, I've sent both versions (atoi v/s strtol), so whichever one
>> is preferred can be used (would be slightly inclined towards atoi, since
>> it's used elsewhere in the file, and because of Clemens' comment;
>> additionally, there is less code duplication with the atoi version).
>>
>> Best regards,
>>
>> On Tue, Jan 29, 2019 at 8:19 AM Clemens Ladisch <clemens@ladisch.de>
>> wrote:
>>
>>> Takashi Sakamoto wrote:
>>> > When parsing string with non-numeric characters, the behaviour of
>>> > atoi() is undefined still in C11.
>>>
>>> A completely non-numeric string is specified to return zero.
>>>
>>> The behaviour is undefined only if the value cannot be represented, i.e.,
>>> if it overflows.
>>>
>>>
>>> Regards,
>>> Clemens
>>>
>>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] seq: arecordmidi: Add num-events option
  2019-02-11  4:23             ` Prashant Malani
@ 2019-02-11  8:14               ` Takashi Iwai
  2019-02-11 10:07                 ` Prashant Malani
  0 siblings, 1 reply; 11+ messages in thread
From: Takashi Iwai @ 2019-02-11  8:14 UTC (permalink / raw)
  To: Prashant Malani; +Cc: alsa-devel, Clemens Ladisch

On Mon, 11 Feb 2019 05:23:36 +0100,
Prashant Malani wrote:
> 
> Friendly ping.

Since no one raised objection, I applied now.


thanks,

Takashi


> On Mon, Feb 4, 2019 at 12:20 PM Prashant Malani <pmalani@chromium.org>
> wrote:
> 
> > Friendly ping. Sounds like Clemens mentioned atoi() should be OK (given
> > his explanation regarding non-numeric strings, along with consistency with
> > the rest of the utility).
> > Are there any other concerns with this patchset?
> >
> > Thanks once again for taking the time to review.
> >
> > Best regards,
> >
> > On Tue, Jan 29, 2019 at 3:42 PM Prashant Malani <pmalani@chromium.org>
> > wrote:
> >
> >> Interesting. Thanks for pointing that out, Clemens.
> >>
> >> In any case, I've sent both versions (atoi v/s strtol), so whichever one
> >> is preferred can be used (would be slightly inclined towards atoi, since
> >> it's used elsewhere in the file, and because of Clemens' comment;
> >> additionally, there is less code duplication with the atoi version).
> >>
> >> Best regards,
> >>
> >> On Tue, Jan 29, 2019 at 8:19 AM Clemens Ladisch <clemens@ladisch.de>
> >> wrote:
> >>
> >>> Takashi Sakamoto wrote:
> >>> > When parsing string with non-numeric characters, the behaviour of
> >>> > atoi() is undefined still in C11.
> >>>
> >>> A completely non-numeric string is specified to return zero.
> >>>
> >>> The behaviour is undefined only if the value cannot be represented, i.e.,
> >>> if it overflows.
> >>>
> >>>
> >>> Regards,
> >>> Clemens
> >>>
> >>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] seq: arecordmidi: Add num-events option
  2019-02-11  8:14               ` Takashi Iwai
@ 2019-02-11 10:07                 ` Prashant Malani
  0 siblings, 0 replies; 11+ messages in thread
From: Prashant Malani @ 2019-02-11 10:07 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, Clemens Ladisch

Thanks Takashi!

On Mon, Feb 11, 2019, 00:15 Takashi Iwai <tiwai@suse.de> wrote:

> On Mon, 11 Feb 2019 05:23:36 +0100,
> Prashant Malani wrote:
> >
> > Friendly ping.
>
> Since no one raised objection, I applied now.
>
>
> thanks,
>
> Takashi
>
>
> > On Mon, Feb 4, 2019 at 12:20 PM Prashant Malani <pmalani@chromium.org>
> > wrote:
> >
> > > Friendly ping. Sounds like Clemens mentioned atoi() should be OK (given
> > > his explanation regarding non-numeric strings, along with consistency
> with
> > > the rest of the utility).
> > > Are there any other concerns with this patchset?
> > >
> > > Thanks once again for taking the time to review.
> > >
> > > Best regards,
> > >
> > > On Tue, Jan 29, 2019 at 3:42 PM Prashant Malani <pmalani@chromium.org>
> > > wrote:
> > >
> > >> Interesting. Thanks for pointing that out, Clemens.
> > >>
> > >> In any case, I've sent both versions (atoi v/s strtol), so whichever
> one
> > >> is preferred can be used (would be slightly inclined towards atoi,
> since
> > >> it's used elsewhere in the file, and because of Clemens' comment;
> > >> additionally, there is less code duplication with the atoi version).
> > >>
> > >> Best regards,
> > >>
> > >> On Tue, Jan 29, 2019 at 8:19 AM Clemens Ladisch <clemens@ladisch.de>
> > >> wrote:
> > >>
> > >>> Takashi Sakamoto wrote:
> > >>> > When parsing string with non-numeric characters, the behaviour of
> > >>> > atoi() is undefined still in C11.
> > >>>
> > >>> A completely non-numeric string is specified to return zero.
> > >>>
> > >>> The behaviour is undefined only if the value cannot be represented,
> i.e.,
> > >>> if it overflows.
> > >>>
> > >>>
> > >>> Regards,
> > >>> Clemens
> > >>>
> > >>
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-02-11 10:08 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-29  1:44 [PATCH 1/1] seq: arecordmidi: Add num-events option pmalani
2019-01-29  3:36 ` Takashi Sakamoto
2019-01-29  5:34   ` Prashant Malani
2019-01-29  8:45     ` Takashi Sakamoto
2019-01-29  9:58       ` pmalani
2019-01-29 16:18       ` Clemens Ladisch
2019-01-29 23:42         ` Prashant Malani
2019-02-04 20:20           ` Prashant Malani
2019-02-11  4:23             ` Prashant Malani
2019-02-11  8:14               ` Takashi Iwai
2019-02-11 10:07                 ` Prashant Malani

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.