On Tuesday 02 December 2003 20:43, Melanie wrote:> Hi, > > I've split it into three patches now and also made it fit to current CVS, > with, I believe, Karl's changes for the double free bug. > > Order for the patches is: > > icecast-multilevel-fallback.patch > icecast-timewindow.patch > icecast-yp-override.patch > > As for docs: How? What format? Any specific layout? > > MelanieMelanie, Since we've now (finally!) got 2.0 out the door, I'm now coming back to these patches - hoping to actually get the functionality incorporated. I've got a few questions, though: 1) Documentation: did you ever manage to write any? The options you added are fairly straightforward for the most part, so this shouldn't be too hard. 2) The 'no-mount' option: why? Does this have any effect at all that isn't handled by setting max-listeners to zero? 3) In the multilevel fallbacks patch, you've added a comment saying "Likely safe to do since there can only be one thread, ever", along with some static local variables, in the source_find_mount() function. I'm not sure why you said this, it's clearly incorrect: the source_* functions are called from many different threads, and this is clearly not safe. This is easily fixed, though, by passing the neccesary variables recursively, and splitting the function into two. I'll probably have more questions once I read the other two patches in detail. Thanks again for your contributions, Mike <p>--- >8 ---- List archives: http://www.xiph.org/archives/ icecast project homepage: http://www.icecast.org/ To unsubscribe from this list, send a message to 'icecast-dev-request@xiph.org' containing only the word 'unsubscribe' in the body. No subject is needed. Unsubscribe messages sent to the list will be ignored/filtered.
On Tuesday 13 January 2004 17:15, Melanie wrote:> Hi, > > On 2004.01.13 02:32 Michael Smith wrote: > > Since we've now (finally!) got 2.0 out the door, I'm now coming back to > > these > > patches - hoping to actually get the functionality incorporated. > > > > I've got a few questions, though: > > 1) Documentation: did you ever manage to write any? The options > > you added are > > fairly straightforward for the most part, so this shouldn't be too hard. > > I put that off until I get some positive confirmation that the patches > _will_ get incorporated. Naturally, I didn't want to waste my time > documenting patches that would never make it into Icecast in the first > place. > However, I'm not refusing to write it, expect some docs when I hear the > patches are definitely going in.That's fair enough. The multilevel fallbacks patch (perhaps with modifications, but certainly with the functionality intact) is definately going in (sometime this week, depending on when I have time). I haven't looked at the others in detail.> > > 2) The 'no-mount' option: why? Does this have any effect at all > > that isn't > > handled by setting max-listeners to zero? > > Fallback will add the clients from the upper level source to the fallback's > pending queue. IIRC, that's where max-listeners gets checked. Setting > max-listeners to zero would disallow any listeners at all. No-mount will > only prevent mounting the source directly, by name, it will not prevent > listening to it as a fallback. That was the idea.The no-mount option gets checked (in your patch) in the same codepath as the (per-mount and global, though global doesn't matter here) max-listeners. Though that indicates a bug somewhere - these (max-listeners and your new no-mount) get checked when adding to the pending queue, but the current number of listeners doesn't get incremented until the source pulls the client off the pending queue. There's no check for either of these parameters when the source pulls the client(s) off the pending queue - so fallbacks are unaffected. So: either no-mount goes (because it's useless), or we change things so that no-mount prevents direct mounts (only), and max-listeners is absolute (and is enforced when the source pulls clients off the pending queue). The latter makes more sense, I guess, but is a significant change in behaviour. Does anyone else have any opinions on this? <p>>> > 3) In the multilevel fallbacks patch, you've added a comment > > saying > > "Likely safe to do since there can only be one thread, ever", along with > > some > > static local variables, in the source_find_mount() function. I'm not sure > > why > > you said this, it's clearly incorrect: the source_* functions are called > > from > > many different threads, and this is clearly not safe. This is easily > > fixed, > > though, by passing the neccesary variables recursively, and splitting the > > function into two. > > AFAIK, checking the function references I found that a certain lock must be > hald to call it, off the top of my head I don't recall which. Since the > function must be called under lock, there should only be one thread az a > time executing it. > I wasn't 100% sure about this, that's why I added the comment, so someone > else would have a look. >Almost right. A lock has to be held (the global.source_tree lock), but that's an rwlock, which allows multiple simultaneous readers. I'll redo the code there. Mike --- >8 ---- List archives: http://www.xiph.org/archives/ icecast project homepage: http://www.icecast.org/ To unsubscribe from this list, send a message to 'icecast-dev-request@xiph.org' containing only the word 'unsubscribe' in the body. No subject is needed. Unsubscribe messages sent to the list will be ignored/filtered.
Hi, On 2004.01.13 02:32 Michael Smith wrote:> Since we've now (finally!) got 2.0 out the door, I'm now coming back to > these > patches - hoping to actually get the functionality incorporated. > > I've got a few questions, though: > 1) Documentation: did you ever manage to write any? The options > you added are > fairly straightforward for the most part, so this shouldn't be too hard.I put that off until I get some positive confirmation that the patches _will_ get incorporated. Naturally, I didn't want to waste my time documenting patches that would never make it into Icecast in the first place. However, I'm not refusing to write it, expect some docs when I hear the patches are definitely going in.> 2) The 'no-mount' option: why? Does this have any effect at all > that isn't > handled by setting max-listeners to zero?Fallback will add the clients from the upper level source to the fallback's pending queue. IIRC, that's where max-listeners gets checked. Setting max-listeners to zero would disallow any listeners at all. No-mount will only prevent mounting the source directly, by name, it will not prevent listening to it as a fallback. That was the idea.> 3) In the multilevel fallbacks patch, you've added a comment > saying > "Likely safe to do since there can only be one thread, ever", along with > some > static local variables, in the source_find_mount() function. I'm not sure > why > you said this, it's clearly incorrect: the source_* functions are called > from > many different threads, and this is clearly not safe. This is easily > fixed, > though, by passing the neccesary variables recursively, and splitting the > function into two.AFAIK, checking the function references I found that a certain lock must be hald to call it, off the top of my head I don't recall which. Since the function must be called under lock, there should only be one thread az a time executing it. I wasn't 100% sure about this, that's why I added the comment, so someone else would have a look. Melanie --- >8 ---- List archives: http://www.xiph.org/archives/ icecast project homepage: http://www.icecast.org/ To unsubscribe from this list, send a message to 'icecast-dev-request@xiph.org' containing only the word 'unsubscribe' in the body. No subject is needed. Unsubscribe messages sent to the list will be ignored/filtered.