Re: sndio: more cleanup + multiple devices support

2011-11-09 Thread Remco
Alexandre Ratchov wrote:

 Initially aucat was designed to handle one device only so multiple
 audio devices used to be supported simply by starting multiple aucat
 processes (one per device), very simple. Then (~1.5 years ago) we
 added support for multiple audio devices (ie multiple -f options), and
 now it's time to finish the job and add a device number component in
 sndio(7) device names. Since this partially breaks compatibility, this
 is a opportunitiy to fix few other design mistakes (eg ':' being used
 by inet6, type name vs api name confusion, etc..). This leads to the
 following names:
 
 type[@hostname][,unit]/devnum[.option]
 

Hi Alexandre,

The highlighted code at the bottom of this mail contains comparisons like
this: strncmp(snd, str, len) == 0

This makes it possible to do things like:
$ aucat -fsnds/0 -i${HOME}/them.wav 
snd0: snds/0: failed to open audio device = Ok, this should fail
$ aucat -fsn/0 -i${HOME}/them.wav = Not Ok ???, this plays the music

The second call doesn't fail because if len is smaller then strlen(snd)
than it potentially matches a partial snd, allowing device types like s
and sn (in the case of type snd). I suppose this is not what you
intended.

I tend to do comparisons like this as follows,
strncmp(str, snd, sizeof(snd)) == 0,
using the NUL character in snd as a sort of boundary check.
I'm not 100% sure though that this is always safe when strlen(str) 
sizeof(snd).

Maybe the safest option is to check if len == strlen(snd) before making
the comparison. This mail includes a PoC patch using this method.

After applying the patch aucat should behave like this:
$ aucat -fsnds/0 -i${HOME}/them.wav 
snd0: snds/0: failed to open audio device
$ aucat -fsnd/0 -i${HOME}/them.wav = Ok, playing music
$ aucat -fsn/0 -i${HOME}/them.wav  
snd0: sn/0: failed to open audio device
$ aucat -fs/0 -i${HOME}/them.wav  
snd0: s/0: failed to open audio device
$ aucat -f/0 -i${HOME}/them.wav  
snd0: /0: failed to open audio device
$ aucat -f0 -i${HOME}/them.wav  
snd0: 0: failed to open audio device

Though I patched mio.c I didn't actually test midicat.
For this proof-of-concept I only tested aucat with device types snd and
rsnd, which seem to work for me.

Some other more or less random weirdness I found is that I can specify either 
sun, sun: or sun:0 for the device and all three of them will play sound.
I'd expect only sun:0 to work. Device aucat hasn't got this problem. I haven't 
tracked this down but I suspect some additional checking is going on outside 
the sio_open function.

P.S.:
I added a comparison function sio_is_type. But I didn't know how to share it 
properly between sio.c and mio.c so my solution might look a bit quick and 
dirty. (especially the extern sio_is_type declaration in mio.c)

 
 Index: lib/libsndio/mio.c
 ===
 RCS file: /cvs/src/lib/libsndio/mio.c,v
 retrieving revision 1.12
 diff -u -p -r1.12 mio.c
 --- lib/libsndio/mio.c17 Oct 2011 21:09:11 -  1.12
 +++ lib/libsndio/mio.c8 Nov 2011 19:18:19 -
 @@ -49,26 +46,23 @@ mio_open(const char *str, unsigned mode,
  if (str == NULL  !issetugid())
  str = getenv(MIDIDEVICE);
  if (str == NULL) {
 - hdl = mio_aucat_open(0, mode, nbio);
 + hdl = mio_aucat_open(/0, mode, nbio, 1);
  if (hdl != NULL)
  return hdl;
 - return mio_rmidi_open(0, mode, nbio);
 + return mio_rmidi_open(/0, mode, nbio);
  }
 - sep = strchr(str, ':');
 - if (sep == NULL) {
 - DPRINTF(mio_open: %s: ':' missing in device name\n, str);
 - return NULL;
 + for (len = 0; (c = str[len]) != '\0'; len++) {
 + /* XXX: remove ':' compat bits */
 + if (c == ':' || c == '@' || c == '/' || c == ',')
 + break;
  }
 - len = sep - str;
 - if (len == (sizeof(prefix_midithru) - 1) 
 - memcmp(str, prefix_midithru, len) == 0)
 - return mio_aucat_open(sep + 1, mode, nbio);
 - if (len == (sizeof(prefix_aucat) - 1) 
 - memcmp(str, prefix_aucat, len) == 0)
 - return mio_aucat_open(sep + 1, mode, nbio);
 - if (len == (sizeof(prefix_rmidi) - 1) 
 - memcmp(str, prefix_rmidi, len) == 0)
 - return mio_rmidi_open(sep + 1, mode, nbio);
 + if (strncmp(snd, str, len) == 0 ||
 + strncmp(aucat, str, len) == 0)
 + return mio_aucat_open(str + len, mode, nbio, 0);
 + if (strncmp(midithru, str, len) == 0)
 + return mio_aucat_open(str + len, mode, nbio, 1);
 + if (strncmp(rmidi, str, len) == 0)
 + return mio_rmidi_open(str + len, mode, nbio);
  DPRINTF(mio_open: %s: unknown device type\n, str);
  return NULL;
  }
 Index: lib/libsndio/sio.c
 ===
 RCS file: /cvs/src/lib/libsndio/sio.c,v
 retrieving revision 1.6
 diff -u -p -r1.6 sio.c
 --- lib/libsndio/sio.c9 May 2011 

Re: sndio: more cleanup + multiple devices support

2011-11-09 Thread Alexandre Ratchov
On Wed, Nov 09, 2011 at 03:14:21PM +0100, Remco wrote:
 
 The highlighted code at the bottom of this mail contains comparisons like
 this: strncmp(snd, str, len) == 0
 

Yes, indeed this part of the code is wrong, grr... second time i do
the very same mistake; thanks

 sun, sun: or sun:0 for the device and all three of them will play
 sound.  I'd expect only sun:0 to work.

I agree, the number is mandatory.

 Device aucat hasn't got this
 problem. I haven't tracked this down but I suspect some additional
 checking is going on outside the sio_open function.

This is one is somewhat intentional. We'll keep the device number
optional until aucat is enabled by default. This is a quick hack to
keep the /dev/audio symlink working for now.

 P.S.:
 I added a comparison function sio_is_type. But I didn't know how to share it 
 properly between sio.c and mio.c so my solution might look a bit quick and 
 dirty. (especially the extern sio_is_type declaration in mio.c)
 

thanks

-- Alexandre



Re: Dell 5540 HSDPA

2011-11-09 Thread Stuart Henderson
On 2011/10/20 10:25, rivo nurges wrote:
 cdce port must be enabled before use by sending following to one
 of the serial ports(should it be somehow documented in the manpage?):
 
 AT+CFUN=1 # enable radio
 AT+CGDCONT=1,IP,internet # configure apn
 AT*ENAP=1,1 # enable cdce port

I've committed your diff - very nice to be able to dhclient cdce0 (and
route add default -ifp cdce0 if it doesn't pick up a route automatically)
rather than mess around making a ppp session to the phone. Now I'm
wondering how far to go with the manpage.

It probably makes sense to at least add something like this in caveats,


Index: cdce.4
===
RCS file: /cvs/src/share/man/man4/cdce.4,v
retrieving revision 1.15
diff -u -p -r1.15 cdce.4
--- cdce.4  9 Nov 2011 21:49:53 -   1.15
+++ cdce.4  9 Nov 2011 21:56:51 -
@@ -144,3 +144,5 @@ Many USB devices notoriously fail to rep
 correctly.
 Undetected products might work flawlessly when their vendor and product IDs
 are added to the driver manually.
+Some mobile data devices must be configured via AT commands on the serial
+interface before they will connect cdce(4) to the mobile network.


It would be handy to list the necessary commands somewhere in the
manual where people can find them easily offline, but I'm not sure
which section would be appropriate for that, maybe at the bottom
of description?

(Actually with my aspireone when I first setup ppp with this card
I had a lot of trouble working out how to get AT+CFUN=1 to report
anything other than ERROR, it took me quite a while to figure out
that I had to flip the 3-way RF-kill slider towards 3G, whereas the
AT commandset list for the device was relatively easy to find ;)