Re: my first patch
On Wed, Oct 25, 2023 at 10:10:32AM +0600, Maria Morisot wrote: > > > that you're using correct lengths though, it is possible to get things > > wrong and break programs. > > I was careful to look at the buffer lengths being written and to match them > in strlcpy and snprintf. I peeked at the source for instances of strcpy and > found a lot in xenocara; less in the main source tree. > > I'm willing to change these but I need to know how to submit the altered > files and since it's my first time contributing, I'd love if someone could > double check a bit of my work. > I think that the explanation of what you are doing and trying to accomplish was a little bit unclear from the responses you got. Upstream means sending your work "up" to the programmers elsewhere who are creating and developing the program. This can allow, if they want to and it doesn't cause problems with other OS's, your changes to be incorporated into the software. This doesn't have anything to do with OpenBSD, except that it will make porting the program into being usable with OpenBSD different. (See ports on the website and the po...@openbsd.org mailing list). So, there is a big difference between changing the original program to work better with OpenBSD or porting it in. Porting in a program means adapting it, possibly with patches that make the changes *only* for OpenBSD. So, Libreoffice isn't an OpenBSD program. Certain patches or changes shouldn't be done locally unless upstream refuses your change. Then, that program will be ported in officially, just marked as broken or dropped as a program for OpenBSD. A really good and simple example is an error in a man page. That sort of change should always be sent upstream first. If they refuse to accept that change, then patch it here. If some flags for our compiler are different, then that is a local adaptation *for OpenBSD*. Do that here and don't bother upstream unless you have questions. Updating programs that are ported in can sometimes be quite difficult when the version changes. That is what Stuart meant about having a nightmare when changing the local copy for us in the way you are doing it. Also, even if everything you have done is 100% perfect, don't be disappointed if your work isn't accepted. Just learn from it and start a new project. OpenBSD has extremely picky and overworked developers. Which is probably why I sleep well at night knowing I have an excellent and secure OS. They do amazingly good work! So even if your first ten tries at different things fail, by the time of your eleventh, you will probably be getting it right by then. Enjoy! -- Chris Bennett
Re: my first patch
Hi Maria, Maria Morisot wrote on Wed, Oct 25, 2023 at 09:44:17AM +0600: > because the compiler said to. Never trust a linter blindly. A linter is a tool to automate searching for some kinds of issues. For every candidate, you still need to engage your brains to figure out - whether it's an actual problem or a false positive; all linters have a non-zero rate of false positives, of varying frequencies - what the importance and priority is - what the best fix is, in general and in particular; even if the linter presents some specific advice, that may still be wrong in the particular case, or even in general - what the risks and costs of attempting a fix are > more than happy to be a code janitor. We don't want code janitors here. Having those is dangerous. It too easily happens that a change is considered "trivial janitorial work", the janitor is too inexperienced to recognize that it is actually wrong or even dangerous, the reviewer is sloppy and spends little time on it because it is just "trivial janitorial work", and on top of that, also because the volume of such seemingly simple changes becomes too massive for proper review, and too tiring. And boom, things go sideways, and nobody notices. Every change, large or small, complicated or seemingly simple, needs to be developed or properly reviewed by at least one developer fully understanding the matter. Large, mechanical diffs that are sometimes typical for janitorial work elsewhere are only accepted round here from developers who are known for their particular experience and diligence. In that sense, janitorial work must clear a *higher* bar here than ordinary commits. > I just need someone to steer me in a direction. Finding something that interests you, that you can already improve in some way right now, or that you want to learn about is an essential part of the job. No one else can do that for you. I have sometimes tried to suggest specific tasks to people who asked for work. But it's a bad idea after all because unless i know someone very well, it's hard to judge what they want and what they can do. The end result often is that i spend more time trying to coach them than it would have taken to complete the task myself - which i didn't have time for in the first place because there are just way too many open tasks. And the success rate of coaching people is *massively* lower than the success rate of doing own work. A good beginner typically has a success rate of maybe 20-30% of the patches they send in to finally get committed after some iterations of taking advice and sending improvements. Beginners with phantastically high success rates on the order of 50-70% occasionally pop up, but they are rare. The majority of people one tries to coach give up before producing anything good enough for commit. For comparison, for doing random work across the tree, developers who are no longer beginners typically have success rates of about 50-70%; within their specialist domain, if any, typically aound 90-98%. Consequently, the success rates for coaching are typically by roughly a factor of ten lower than for own work, and often require more working time per successful commit on top of that. All these numbers are personal estimates from anecdotal observation, i did not attempt empirical reaseach into these matters. > I'll keep plugging away at my own fancy Not such a bad idea actually. If you are energetic and curious, it's hard to believe you aren't already aware of at least some quirks that irk you. If you watch the mailing lists, you will see plenty of potential problems that others report. Even if you judge many of them too hard for your current knowledge and experience level, if you are intelligent, you are likely able to determine root causes and develop fixes or improvements for some others. In any case, don't be pessimistic. Having time and motivation at your disposal is not a bad starting point for learning and for doing work that interests you. Being aware of personal weaknesses - anybody has some, so generally nobody ought to regard them as roadblocks - and using your personal strengths to your profit and contentment is also a good idea. Good luck and have fun, Ingo
Re: my first patch
I thought xenocara was kind of incorporated into the OpenBSD project. Thanks for responding. I think when I started out a month ago, my main issue was having hangups in the kernel from running xfce. I've since migrated to cwm and am much happier than when using xfce on Linux. I have high energy, doctors call it mania; my peers call it aptitude for prolific output. I just need someone to steer me in a direction. What needs work and what do I need to learn. I am technically disabled with mental illness, so I have income and don't need to work. I have near mensa intelligence. I've read computer books for 20 years on various topics, but it's scattered. If nobody will give me direction then I'll keep plugging away at my own fancy and probably annoy everyone on the list until I finally get kicked off. /story of my life. -- Google doesn't need to know every time I fart. > On Oct 25, 2023, at 14:04, Stuart Henderson wrote: > > On 2023-10-25, Maria Morisot wrote: >> Basically I just changed all instances of strcpy and sprintf to use strlcpy >> and snprintf, because the compiler said to. >>> >>> This sort of change should go upstream rather than in ports. Be careful >>> that you're using correct lengths though, it is possible to get things >>> wrong and break programs. >> >> What does upstream mean, in the main source tree? I would have guessed that >> such changes were already implemented in the main system. > > If you're looking at code directly in an OpenBSD CVS repository and it > still has strcpy/sprintf, it almost certainly does not originate from > OpenBSD itself, rather comes from another origin, and often updated from > time to time. Examples of this include most of the xenocara tree for > X, some of the toolchain used to produce binaries (compilers, linkers > and so on) and a few other parts of the system. Making changes like > this to such code in the OpenBSD tree makes it difficult to update to > newer versions; if done at all, it's usually done "the other way round" > - changed in the origin, and then synced across as part of a later update > to OpenBSD. > > Similarly for software in the ports tree, carrying patches for things > like this often makes it difficult to update to a newer version. > They're usually quite intrusive and affecting large parts of the source > distribution. > > And while they can be simple, strcpy/sprintf/similar conversions still > need careful review, to make sure that there aren't off-by-1 calculation > errors in string length, or to make sure that actual string lengths are > used and not e.g. size of a char * pointer. > > Generally I'd suggest that someone new to this finds problems/niggles in > their use of the system, looks at how it works and figures out what's > going on and what changes could be made to improve that. There's a lower > risk of accidentally introducing problems if you're looking at code > which you actually use rather than in a big sweep of the source tree, > and probably better for gaining understanding. > > Also if there's some code which interests you, you might like to try > seeing how it changed over time, how problems were fixed, etc (i.e. read > the commit history - this may be simpler by using the mirror on github > than cvs, as you can see changes grouped together as a whole commit, > which can include changes to multiple files at once). A big part of > working on software like this is understanding the history and why > things were done. > > > -- > Please keep replies on the mailing list. >
Re: my first patch
On 2023-10-25, Maria Morisot wrote: > Basically I just changed all instances of strcpy and sprintf to use strlcpy > and snprintf, because the compiler said to. >> >> This sort of change should go upstream rather than in ports. Be careful >> that you're using correct lengths though, it is possible to get things >> wrong and break programs. > > What does upstream mean, in the main source tree? I would have guessed that > such changes were already implemented in the main system. If you're looking at code directly in an OpenBSD CVS repository and it still has strcpy/sprintf, it almost certainly does not originate from OpenBSD itself, rather comes from another origin, and often updated from time to time. Examples of this include most of the xenocara tree for X, some of the toolchain used to produce binaries (compilers, linkers and so on) and a few other parts of the system. Making changes like this to such code in the OpenBSD tree makes it difficult to update to newer versions; if done at all, it's usually done "the other way round" - changed in the origin, and then synced across as part of a later update to OpenBSD. Similarly for software in the ports tree, carrying patches for things like this often makes it difficult to update to a newer version. They're usually quite intrusive and affecting large parts of the source distribution. And while they can be simple, strcpy/sprintf/similar conversions still need careful review, to make sure that there aren't off-by-1 calculation errors in string length, or to make sure that actual string lengths are used and not e.g. size of a char * pointer. Generally I'd suggest that someone new to this finds problems/niggles in their use of the system, looks at how it works and figures out what's going on and what changes could be made to improve that. There's a lower risk of accidentally introducing problems if you're looking at code which you actually use rather than in a big sweep of the source tree, and probably better for gaining understanding. Also if there's some code which interests you, you might like to try seeing how it changed over time, how problems were fixed, etc (i.e. read the commit history - this may be simpler by using the mirror on github than cvs, as you can see changes grouped together as a whole commit, which can include changes to multiple files at once). A big part of working on software like this is understanding the history and why things were done. -- Please keep replies on the mailing list.
Re: my first patch
On Tue, Oct 24, 2023 at 10:42:06PM +0200, Jan Stary wrote: > On Oct 24 22:09:02, a...@caoua.org wrote: > > faad -w file.m4a | cat >file.wav > > results in a file with zero-size data chunk (because faad couldn't > > seek to the beginning of the file to fixup the header). aucat, > > audacious, audacity and sox can't play it; mpv, and ffplay can > > SoX's play --ignore-length can play it. > excellent! thank you
Re: my first patch
On Tue, Oct 24, 2023 at 11:11:01PM +0600, Maria Morisot wrote: > > Forgive me if I'm dense, but I'm a better artist than I am a > programmer. I'm trying to follow you though. I understand why you > cannot seek in a pipe, but I do not understand why that affects > playback of a MS Wav file through a pipe. There are two reasons: First, the .wav file may have many headers, in any order, and one data chunk. aucat has to parse certain headers (ex. to extract the data format and its size) and skip others (unused meta data). Currently, it uses lseek(2) to move the file pointer from one header to another and to move to the data location to start reading. This is how .wav files need to be handled, and as lseek(2) doesn't work on pipes aucat can't read .wav files from a pipe. However, on many simple .wav files the lseek(2) call is a no-op because there's no header to skip. Furthermore, to skip small headers and/or padding, read(2) could be used instead of lseek(2), which works on pipes. There's a second problem. The .wav file has a header that contains the data chunk size. Programs generating the .wav file, like faad, don't know the decoded data size in advance. So, they write the header with zeroed data size field, then write the data chunk, and finally lseek(2) to the header to fixup the data size field. This can't work with pipes because of the lseek(2) call and the files end up with an invalid header (zero data chunk size). Certain programs interpret zero as "until the end of the file" (mpv, ffmpeg) and manage to play such files, other as don't. The diff in the other mail addresses these two problems. > Aren't the headers kept in > the front, and my understanding is maybe you can grab enough bytes > to check if a header is present. I thought wav is just a raw sample > with a small header. I'd think a quick check for header wouldn't > upset playback for raw samples without one. Yes, this would be nice to have, to avoid using -h
Re: my first patch
On Oct 25 12:10:07, maria.mori...@icloud.com wrote: > > > SoX's play --ignore-length can play it. > > > SoX's --ignore-length appears in a few formats, as far as the wav format > goes, the code comments suggest it is implemented to handle 32-bit wav files > greater than 2GB. It seems you just found a happy side effect. --ignore-length Override an (incorrect) audio length given in an audio file's header. If this option is given then SoX will keep reading audio until it reaches the end of the input file.
Re: my first patch
I'll check out SoX's --ignore-length option and see if I can figure out what they are doing there. What I'm more concerned with than the implementation is which behavior seems more correct; to try to play at the specified rate or to just ignore and play at the default settings. -- Google doesn't need to know every time I fart. > On Oct 25, 2023, at 02:42, Jan Stary wrote: > > On Oct 24 22:09:02, a...@caoua.org wrote: >> faad -w file.m4a | cat >file.wav >> results in a file with zero-size data chunk (because faad couldn't >> seek to the beginning of the file to fixup the header). aucat, >> audacious, audacity and sox can't play it; mpv, and ffplay can > > SoX's play --ignore-length can play it. >
Re: my first patch
> SoX's play --ignore-length can play it. > SoX's --ignore-length appears in a few formats, as far as the wav format goes, the code comments suggest it is implemented to handle 32-bit wav files greater than 2GB. It seems you just found a happy side effect.
Re: my first patch
> that you're using correct lengths though, it is possible to get things > wrong and break programs. I was careful to look at the buffer lengths being written and to match them in strlcpy and snprintf. I peeked at the source for instances of strcpy and found a lot in xenocara; less in the main source tree. I'm willing to change these but I need to know how to submit the altered files and since it's my first time contributing, I'd love if someone could double check a bit of my work.
Re: my first patch
Basically I just changed all instances of strcpy and sprintf to use strlcpy and snprintf, because the compiler said to. > > This sort of change should go upstream rather than in ports. Be careful > that you're using correct lengths though, it is possible to get things > wrong and break programs. What does upstream mean, in the main source tree? I would have guessed that such changes were already implemented in the main system. I just want to be a productive member and my programming experience is limited. If someone can point me to work that makes a difference doing menial tasks, I'd be more than happy to be a code janitor.
Re: my first patch
On Oct 24 22:09:02, a...@caoua.org wrote: > faad -w file.m4a | cat >file.wav > results in a file with zero-size data chunk (because faad couldn't > seek to the beginning of the file to fixup the header). aucat, > audacious, audacity and sox can't play it; mpv, and ffplay can SoX's play --ignore-length can play it.
Re: my first patch
On 2023-10-24, Lucretia wrote: > I made my first patch! > > To devel/dwz, I'm not sure how to submit it, or if it's even useful to anyone. > > Basically I just changed all instances of strcpy and sprintf to use strlcpy > and snprintf, because the compiler said to. This sort of change should go upstream rather than in ports. Be careful that you're using correct lengths though, it is possible to get things wrong and break programs.
Re: my first patch
On Wed, Oct 25, 2023 at 12:06:05AM +0600, Maria Morisot wrote: > > I don't have a test machine and I'm trying to keep my installation > as simple as possible, but if anyone wants to try piping a wav file > into mplayer or ffplay, I'd be interested in the results. Does it > work? faad -o file.wav file.m4a results in a file.wav that aucat and most players can play. faad -w file.m4a | cat >file.wav results in a file with zero-size data chunk (because faad couldn't seek to the beginning of the file to fixup the header). aucat, audacious, audacity and sox can't play it; mpv, and ffplay can Here's a diff for aucat to cope with such files: - if the header indicates zero-size data chunk, try to play the data until the end of the file is reached - if there are small sections to skip (padding for alignement or meta info), then read it instead of using lseek(2) - short reads are allowed for pipes, so when reading the headers, retry if needed. please test Index: afile.c === RCS file: /cvs/src/usr.bin/aucat/afile.c,v diff -u -p -r1.12 afile.c --- afile.c 27 Mar 2023 15:36:18 - 1.12 +++ afile.c 24 Oct 2023 20:05:30 - @@ -217,14 +217,60 @@ be32_set(be32_t *p, unsigned int v) } static int +afile_readseg(struct afile *f, void *addr, size_t size) +{ + ssize_t n; + + /* +* retry as pipes may return fewer bytes than requested +*/ + while (size > 0) { + n = read(f->fd, addr, size); + if (n == 0 || n == -1) + return 0; + addr = (char *)addr + n; + size -= n; + f->curpos += n; + } + return 1; +} + +static int +afile_setpos(struct afile *f, off_t pos) +{ + static char unused[512]; + off_t off = pos - f->curpos; + + /* +* seek only if needed (to avoid errors with pipes) +*/ + if (off != 0) { + /* +* to skip few bytes only (padding, meta-info), simply read +* them instead of using lseek(2) +*/ + if (off > 0 && off <= sizeof(unused)) { + log_puts("reading\n"); + return afile_readseg(f, unused, off); + } + + log_puts("seeking\n"); + if (lseek(f->fd, pos, SEEK_SET) == -1) + return 0; + f->curpos = pos; + } + return 1; +} + +static int afile_readhdr(struct afile *f, void *addr, size_t size) { - if (lseek(f->fd, 0, SEEK_SET) == -1) { + if (!afile_setpos(f, 0)) { log_puts(f->path); log_puts(": failed to seek to beginning of file\n"); return 0; } - if (read(f->fd, addr, size) != size) { + if (!afile_readseg(f, addr, size)) { log_puts(f->path); log_puts(": failed to read header\n"); return 0; @@ -301,7 +347,7 @@ afile_wav_readfmt(struct afile *f, unsig } if (csize > WAV_FMT_EXT_SIZE) csize = WAV_FMT_EXT_SIZE; - if (read(f->fd, &fmt, csize) != csize) { + if (!afile_readseg(f, &fmt, csize)) { log_puts(f->path); log_puts(": failed to read format chunk\n"); return 0; @@ -377,7 +423,7 @@ afile_wav_readhdr(struct afile *f) log_puts(": missing data chunk\n"); return 0; } - if (read(f->fd, &chunk, sizeof(chunk)) != sizeof(chunk)) { + if (!afile_readseg(f, &chunk, sizeof(chunk))) { log_puts(f->path); log_puts(": failed to read chunk header\n"); return 0; @@ -389,7 +435,15 @@ afile_wav_readhdr(struct afile *f) fmt_done = 1; } else if (memcmp(chunk.id, wav_id_data, 4) == 0) { f->startpos = pos + sizeof(riff) + sizeof(chunk); - f->endpos = f->startpos + csize; + if (csize > 0) + f->endpos = f->startpos + csize; + else { + if (log_level >= 2) { + log_puts(f->path); + log_puts(": reading to end-fo-file\n"); + } + f->endpos = -1; /* read until EOF */ + } break; } else { #ifdef DEBUG @@ -404,7 +458,7 @@ afile_wav_readhdr(struct afile *f) * next chunk */ pos += sizeof(struct wav_chunk) + csize; - if (lseek(f->fd, sizeof(riff) + pos, SEEK_SET) == -1) { + if (!afile_setpos(f, sizeof(riff) + pos)) { log
Re: my first patch
I don't have a test machine and I'm trying to keep my installation as simple as possible, but if anyone wants to try piping a wav file into mplayer or ffplay, I'd be interested in the results. Does it work?
Re: my first patch
> You're right. The .wav headers require to lseek(2) within the file > which doesn't work on a pipes. It could work on certain files which > headers are placed in a way lseek(2) doesn't need to move the file > pointer. > You could try to modify aucat to skip the lseek(2) calls if it > wouldn't change the file pointer. Possibly call read(2) and discard > few bytes when the file pointer moves forward by few bytes only (iirc > .wav needs data to be aligned). Forgive me if I'm dense, but I'm a better artist than I am a programmer. I'm trying to follow you though. I understand why you cannot seek in a pipe, but I do not understand why that affects playback of a MS Wav file through a pipe. Aren't the headers kept in the front, and my understanding is maybe you can grab enough bytes to check if a header is present. I thought wav is just a raw sample with a small header. I'd think a quick check for header wouldn't upset playback for raw samples without one.
Re: my first patch
On Tue, Oct 24, 2023 at 09:15:57PM +0600, Maria Morisot wrote: > It is my understanding that wav files contain the headers necessary for a > program to adjust the audio settings for play, or to do the software process > necessary to reformat the input to the audio device. > > It doesn't make sense to have the wav headers if they aren't going to be > used. Tell me if I'm wrong. > > I'm not very good at C but I'm willing to try to fix aucat to adjust wav > output in response to the headers if that's something that seems like it's > broken. > You're right. The .wav headers require to lseek(2) within the file which doesn't work on a pipes. It could work on certain files which headers are placed in a way lseek(2) doesn't need to move the file pointer. You could try to modify aucat to skip the lseek(2) calls if it wouldn't change the file pointer. Possibly call read(2) and discard few bytes when the file pointer moves forward by few bytes only (iirc .wav needs data to be aligned). HTH
Re: my first patch
It is my understanding that wav files contain the headers necessary for a program to adjust the audio settings for play, or to do the software process necessary to reformat the input to the audio device. It doesn't make sense to have the wav headers if they aren't going to be used. Tell me if I'm wrong. I'm not very good at C but I'm willing to try to fix aucat to adjust wav output in response to the headers if that's something that seems like it's broken. But I did find an alternate solution. I just "-o /dev/audio0" in faad, and use "-f 2" (raw pcm); this works and seems to play at 44100 because it sounds good to me. Not sure how to check default playrate or change it via command line, so this works out of the box: gethsemane$ faad -f 2 -o /dev/audio Tori_Amos/The_Beekeeper/03*.m4a -- Code is poetry. > On Oct 24, 2023, at 21:03, Alexandre Ratchov wrote: > > On Tue, Oct 24, 2023 at 05:10:53PM +0600, Lucretia wrote: >> >> a bit off-topic, but: >> gethsemane$ faad -w Tori_Amos/The_Beekeeper/03* | aucat -i - -h wav >> makes Tori sound like Minnie Mouse. How can I fix this? >> > > you've make faad and aucat use the same data format, ex: > > faad -d -f2 -w foobar.m4a | aucat -e s16 -i - > > possibly use the -r option if the rate is not 48kHz (which is aucat > default). Alternatively, output in a temporary .wav file and play it > after it's decoded
Re: my first patch
On Tue, Oct 24, 2023 at 05:10:53PM +0600, Lucretia wrote: > > a bit off-topic, but: > gethsemane$ faad -w Tori_Amos/The_Beekeeper/03* | aucat -i - -h wav > makes Tori sound like Minnie Mouse. How can I fix this? > you've make faad and aucat use the same data format, ex: faad -d -f2 -w foobar.m4a | aucat -e s16 -i - possibly use the -r option if the rate is not 48kHz (which is aucat default). Alternatively, output in a temporary .wav file and play it after it's decoded
my first patch
I made my first patch! To devel/dwz, I'm not sure how to submit it, or if it's even useful to anyone. Basically I just changed all instances of strcpy and sprintf to use strlcpy and snprintf, because the compiler said to. This is like crack cocaine to me. a bit off-topic, but: gethsemane$ faad -w Tori_Amos/The_Beekeeper/03* | aucat -i - -h wav makes Tori sound like Minnie Mouse. How can I fix this? -- Google doesn't need to know every time I fart.