Josh Lehan wrote:
+++ b/amidicat/amidicat.c +#define MAX_HEX_DIGITS (2)
Is this symbol defined for future compatibility with larger bytes? ;-)
- int is_hex;
For booleans, use "bool" from <stdbool.h>.
+prettyprint_capmask(unsigned int capmask) +{
- /* The r/w letters indicate read/write capability */
- /* Capital R/W letters indicate subscription */
Would anybody understand the difference?
Just restrict the listing to ports that you can handle.
port_name = strdup(snd_seq_port_info_get_name(port_info));
Please note that checkpatch is just a tool to find problems; introducing new problems like this just to shut it up is counterproductive.
(Overlong lines could be a sign that the nesting is too deep, but the ALSA function names do not help ...)
+str_to_cli_port(snd_seq_t *handle, char *str, int *outcli, int *outport)
Why not use snd_seq_parse_address()?
- r = snd_seq_set_client_name(handle, cli_name);
- if (r < 0) {
/* This early in the program, it's not threaded,* errno is OK to use */fprintf(stderr, "Unable to set ALSA client name: %s\n",strerror(errno));
The error code is in r. Use snd_strerror().
(And errno _is_ thread safe.)
- /* FUTURE: Do we need any more type bits here? */
- port_id = snd_seq_create_simple_port(handle, cli_name, caps,
SND_SEQ_PORT_TYPE_MIDI_GENERIC);
You should set all appropriate TYPE bits. You must set CAP bits if you want to allow others to connect from/to this port.
r = snd_seq_event_output_direct(handle, ev);if (r < 0) {/* Return if a real error happened */if (-EAGAIN != r)
EAGAIN can never happen if the device has not been configured to be non-blocking.
r = snd_seq_drain_output(handle);
snd_seq_event_output_direct() bypasses the output buffer, so this does not make sense.
r = snd_seq_sync_output_queue(handle);
You have not scheduled any events to be delivered later, so this does not make sense.
/* FUTURE: Why does snd_seq_sync_output_queue()* always return 1, not 0 as it should? */
Because the documentation is wrong.
r = printf("%02X%s", ui,((size_left > 1) ? " " : "\n"));
Why not use %c?
/* Standard C whitespace *//* Avoid usage of isspace()* because that would introduce locale variations */
Where does this program change the locale to be different from "C"? (And what would it matter if a Klingon space were to be ignored? :-)
/* Send a dummy message to ourself,* so ALSA gets unblocked in other thread */snd_seq_ev_clear(&ev);
This results in a SND_SEQ_EVENT_SYSTEM event. You might want to use one of the USR events ...
/* Check global flag, under lock,* see if other thread is telling us to exit */pthread_mutex_lock(&g_mutex);is_endinput = g_is_endinput;pthread_mutex_unlock(&g_mutex);
... and if you check for that event (and its source address) here, you don't need a separate mutex.
- /* Apologies for the awkward line breaks,
* the mandate of the checkpatch script left no alternative */
checkpatch looks for printk(). Ignore it.
- pthread_cond_init(&g_cond, NULL);
This is not used.
Regards, Clemens