[PATCH] Add mp_strings.c with mp_asprintf function.
Hi, I just made a small equivalent to the GNU asprintf function (since I suppose it's not available on all system supported by mplayer) in order to use it in a few places instead of fixed size buffers like BUFLENGTH in some vo (jpeg, png, …) and maybe a few other PATH_MAX we were speaking a while ago. I made a new file mp_strings.c to store it, maybe we can move later a few other string processing functions (I'm thinking of subtitles text processing) in it. If the name isn't appropriate or there is already a better place to put it, just let me know. Regards, -- Clément B.
On Wed, Jan 12, 2011 at 11:44:31PM +0100, Clément Bœsch wrote:
[...] Subject: [PATCH] Add mp_strings.c with mp_asprintf function.
I'll wait 3 more days for this one. Btw, it's not in use atm so there is no harm. This will allow fixing a lot of buffer issues like I already said. But maybe there is a simpler way (using talloc just like Uoti choose in his tree). Whatever the solution, something must be done about this. Please share your opinion. Regards, -- Clément B.
On Wed, Jan 12, 2011 at 11:44:32PM +0100, Clément Bœsch wrote:
I just made a small equivalent to the GNU asprintf function (since I suppose it's not available on all system supported by mplayer) in order to use it in a few places instead of fixed size buffers like BUFLENGTH in some vo (jpeg, png, …) and maybe a few other PATH_MAX we were speaking a while ago.
Well, I wonder which case you think is worth the effort. PATH_MAX + stack isn't so great but it's not such a big issue, and actually it may be a smaller issue that using this method and ending up bringing the PC to its knees swapping because someone by accident gave a 2GB file as playlist and MPlayer tries to actually use a 2GB string as file name...
On Mon, Jan 17, 2011 at 09:08:32PM +0100, Reimar Döffinger wrote:
On Wed, Jan 12, 2011 at 11:44:32PM +0100, Clément Bœsch wrote:
I just made a small equivalent to the GNU asprintf function (since I suppose it's not available on all system supported by mplayer) in order to use it in a few places instead of fixed size buffers like BUFLENGTH in some vo (jpeg, png, …) and maybe a few other PATH_MAX we were speaking a while ago.
Well, I wonder which case you think is worth the effort. PATH_MAX + stack isn't so great but it's not such a big issue,
I thought about using PATH_MAX in a few places, but under Linux (at least here), it is set to 4096, which make quiet a big buffer for nothing… And even though, it may not be enough sometimes while in most case it is too much.
and actually it may be a smaller issue that using this method and ending up bringing the PC to its knees swapping because someone by accident gave a 2GB file as playlist and MPlayer tries to actually use a 2GB string as file name...
This is imo a special case which could be handled differently, and we can still use PATH_MAX in some places. The point is that it may be a good helper in a few places. And we could use this function in a few other places, for example all the manual snprintf workaround we recently made regarding the http streaming and proxy. -- Clément B.
On Wed, Jan 12, 2011 at 11:44:32PM +0100, Clément Bœsch wrote:
I just made a small equivalent to the GNU asprintf function (since I suppose it's not available on all system supported by mplayer) in order to use it in a few places instead of fixed size buffers like BUFLENGTH in some vo (jpeg, png, …) and maybe a few other PATH_MAX we were speaking a while ago.
The traditional place for this is the osdep/ directory. You should also check in configure for its presence.
--- /dev/null +++ b/mp_strings.c @@ -0,0 +1,49 @@ + +#include <stdlib.h> +#include <stdarg.h> +#include <stdio.h> +#include "mp_strings.h"
nit: It's nice to separate system and local header #include by an empty line.
--- /dev/null +++ b/mp_strings.h @@ -0,0 +1,26 @@ + +#ifndef MP_STRINGS_H +# define MP_STRINGS_H + +int mp_asprintf(char **strp, const char *fmt, ...); + +#endif
#ifndef MPLAYER_MP_STRINGS_H #define MPLAYER_MP_STRINGS_H #endif /* MPLAYER_MP_STRINGS_H */ Diego
On Tue, Jan 18, 2011 at 09:43:55AM +0100, Diego Biurrun wrote:
On Wed, Jan 12, 2011 at 11:44:32PM +0100, Clément Bœsch wrote:
I just made a small equivalent to the GNU asprintf function (since I suppose it's not available on all system supported by mplayer) in order to use it in a few places instead of fixed size buffers like BUFLENGTH in some vo (jpeg, png, …) and maybe a few other PATH_MAX we were speaking a while ago.
The traditional place for this is the osdep/ directory. You should also check in configure for its presence.
I would have done it in case the function were called "asprintf", and as pointed out on IRC, it would need a configure test, conditionally compilation, etc. which is a small burden. So there is a few solutions: - Keep the current patch untouched - Make a osdep/asprintf only compiled on non-gnu systems - Add some ifdef in the function to call local asprintf if GNU system or else this code (and put it in osdep/?) - Use talloc as pointed out previously (it has a talloc_asprintf iirc) If anyone has opinion on this, please share.
--- /dev/null +++ b/mp_strings.c @@ -0,0 +1,49 @@ + +#include <stdlib.h> +#include <stdarg.h> +#include <stdio.h> +#include "mp_strings.h"
nit: It's nice to separate system and local header #include by an empty line.
Fixed locally.
--- /dev/null +++ b/mp_strings.h @@ -0,0 +1,26 @@ + +#ifndef MP_STRINGS_H +# define MP_STRINGS_H + +int mp_asprintf(char **strp, const char *fmt, ...); + +#endif
#ifndef MPLAYER_MP_STRINGS_H #define MPLAYER_MP_STRINGS_H
#endif /* MPLAYER_MP_STRINGS_H */
ditto. -- Clément B.
On Thu, Jan 20, 2011 at 09:25:45PM +0100, Clément Bœsch wrote:
On Tue, Jan 18, 2011 at 09:43:55AM +0100, Diego Biurrun wrote:
On Wed, Jan 12, 2011 at 11:44:32PM +0100, Clément Bœsch wrote:
I just made a small equivalent to the GNU asprintf function (since I suppose it's not available on all system supported by mplayer) in order to use it in a few places instead of fixed size buffers like BUFLENGTH in some vo (jpeg, png, …) and maybe a few other PATH_MAX we were speaking a while ago.
The traditional place for this is the osdep/ directory. You should also check in configure for its presence.
I would have done it in case the function were called "asprintf", and as pointed out on IRC, it would need a configure test, conditionally compilation, etc. which is a small burden.
It's easy enough to do. If you need help, ask. Diego
On 20 Jan 2011, at 21:48, Diego Biurrun <diego@biurrun.de> wrote:
On Thu, Jan 20, 2011 at 09:25:45PM +0100, Clément Bœsch wrote:
On Tue, Jan 18, 2011 at 09:43:55AM +0100, Diego Biurrun wrote:
On Wed, Jan 12, 2011 at 11:44:32PM +0100, Clément Bœsch wrote:
I just made a small equivalent to the GNU asprintf function (since I suppose it's not available on all system supported by mplayer) in order to use it in a few places instead of fixed size buffers like BUFLENGTH in some vo (jpeg, png, …) and maybe a few other PATH_MAX we were speaking a while ago.
The traditional place for this is the osdep/ directory. You should also check in configure for its presence.
I would have done it in case the function were called "asprintf", and as pointed out on IRC, it would need a configure test, conditionally compilation, etc. which is a small burden.
It's easy enough to do. If you need help, ask.
I have some concerns that this might also cause some issues when trying to move to std=c99. Also having our own wrapper means we can add things like limiting the allocation size in the function instead of having a check before each call.
On Fri, Jan 21, 2011 at 07:52:20AM +0100, Reimar Döffinger wrote:
On 20 Jan 2011, at 21:48, Diego Biurrun <diego@biurrun.de> wrote:
On Thu, Jan 20, 2011 at 09:25:45PM +0100, Clément Bœsch wrote:
On Tue, Jan 18, 2011 at 09:43:55AM +0100, Diego Biurrun wrote:
On Wed, Jan 12, 2011 at 11:44:32PM +0100, Clément Bœsch wrote:
I just made a small equivalent to the GNU asprintf function (since I suppose it's not available on all system supported by mplayer) in order to use it in a few places instead of fixed size buffers like BUFLENGTH in some vo (jpeg, png, …) and maybe a few other PATH_MAX we were speaking a while ago.
The traditional place for this is the osdep/ directory. You should also check in configure for its presence.
I would have done it in case the function were called "asprintf", and as pointed out on IRC, it would need a configure test, conditionally compilation, etc. which is a small burden.
It's easy enough to do. If you need help, ask.
I have some concerns that this might also cause some issues when trying to move to std=c99.
What kind of issue?
Also having our own wrapper means we can add things like limiting the allocation size in the function instead of having a check before each call.
As you wish. Here is a second patch with asprintf tested at configure time, choose whatever you prefer. Note on this second patch: I can't test it properly, but I mostly based the code on the strsep code so it should do the trick. If anyone can test it on a non-gnu system, please test if osdep/asprintf.c is well built. Note 2: maybe there is some kind of helper to simplify the test, and still not in use for the strsep code, but I didn't find one. If there is, I'll of course update the code. -- Clément B.
On Fri, Jan 21, 2011 at 03:28:33PM +0100, Clément Bœsch wrote:
As you wish. Here is a second patch with asprintf tested at configure time, choose whatever you prefer.
Note on this second patch: I can't test it properly, but I mostly based the code on the strsep code so it should do the trick. If anyone can test it on a non-gnu system, please test if osdep/asprintf.c is well built.
Just rename the function to asprintf2, detection will fail and you can test if everything still works properly.
Note 2: maybe there is some kind of helper to simplify the test, and still not in use for the strsep code, but I didn't find one. If there is, I'll of course update the code.
--- a/configure +++ b/configure @@ -3811,6 +3811,19 @@ fi
+echocheck "asprintf()" +_asprintf=no +statement_check stdio.h 'char *s; asprintf(&s, "%s:%d", "hello", 0x2a)' && _asprintf=yes +if test "$_asprintf" = yes ; then
Please use just "asprintf" as variable name, I plan to eventually drop all leading underscores.
--- /dev/null +++ b/osdep/asprintf.c @@ -0,0 +1,50 @@ +/* + * asprintf implementation for non-gnu systems
nit: GNU No comment about the implementation; I haven't even read the asprintf man page yet.
--- /dev/null +++ b/osdep/asprintf.h @@ -0,0 +1,26 @@ +/* + * asprintf implementation for non-gnu systems
ditto Diego
On Fri, Jan 21, 2011 at 03:28:33PM +0100, Clément Bœsch wrote:
On Fri, Jan 21, 2011 at 07:52:20AM +0100, Reimar Döffinger wrote:
On 20 Jan 2011, at 21:48, Diego Biurrun <diego@biurrun.de> wrote:
On Thu, Jan 20, 2011 at 09:25:45PM +0100, Clément Bœsch wrote:
On Tue, Jan 18, 2011 at 09:43:55AM +0100, Diego Biurrun wrote:
On Wed, Jan 12, 2011 at 11:44:32PM +0100, Clément Bœsch wrote:
I just made a small equivalent to the GNU asprintf function (since I suppose it's not available on all system supported by mplayer) in order to use it in a few places instead of fixed size buffers like BUFLENGTH in some vo (jpeg, png, …) and maybe a few other PATH_MAX we were speaking a while ago.
The traditional place for this is the osdep/ directory. You should also check in configure for its presence.
I would have done it in case the function were called "asprintf", and as pointed out on IRC, it would need a configure test, conditionally compilation, etc. which is a small burden.
It's easy enough to do. If you need help, ask.
I have some concerns that this might also cause some issues when trying to move to std=c99.
What kind of issue?
Sorry for the late reply. What I would expect might happen is that in C99 mode asprintf is not available (and we certainly don't want to have yet another thing that requires _GNU_SOURCE everywhere), so configure detection will fail. However linking will fail as well, since then asprintf is defined both in the libc and in MPlayer.
On Wed, Jan 26, 2011 at 07:21:49PM +0100, Reimar Döffinger wrote:
On Fri, Jan 21, 2011 at 03:28:33PM +0100, Clément Bœsch wrote:
On Fri, Jan 21, 2011 at 07:52:20AM +0100, Reimar Döffinger wrote:
On 20 Jan 2011, at 21:48, Diego Biurrun <diego@biurrun.de> wrote:
On Thu, Jan 20, 2011 at 09:25:45PM +0100, Clément Bœsch wrote:
On Tue, Jan 18, 2011 at 09:43:55AM +0100, Diego Biurrun wrote:
On Wed, Jan 12, 2011 at 11:44:32PM +0100, Clément Bœsch wrote: > > I just made a small equivalent to the GNU asprintf function (since I > suppose it's not available on all system supported by mplayer) in order to > use it in a few places instead of fixed size buffers like BUFLENGTH in > some vo (jpeg, png, …) and maybe a few other PATH_MAX we were speaking a > while ago.
The traditional place for this is the osdep/ directory. You should also check in configure for its presence.
I would have done it in case the function were called "asprintf", and as pointed out on IRC, it would need a configure test, conditionally compilation, etc. which is a small burden.
It's easy enough to do. If you need help, ask.
I have some concerns that this might also cause some issues when trying to move to std=c99.
What kind of issue?
Sorry for the late reply. What I would expect might happen is that in C99 mode asprintf is not available (and we certainly don't want to have yet another thing that requires _GNU_SOURCE everywhere), so configure detection will fail. However linking will fail as well, since then asprintf is defined both in the libc and in MPlayer.
I'm not sure I get the difference between the compilation test made at ./configure and the way mplayer is built; the asprintf test does not check if we have _GNU_SOURCE defined, it try to compile a source code using it. Or did I missed something again? -- Clément B.
On Wed, Jan 26, 2011 at 04:17:08PM +0100, Diego Biurrun wrote:
On Fri, Jan 21, 2011 at 03:28:33PM +0100, Clément Bœsch wrote:
As you wish. Here is a second patch with asprintf tested at configure time, choose whatever you prefer.
Note on this second patch: I can't test it properly, but I mostly based the code on the strsep code so it should do the trick. If anyone can test it on a non-gnu system, please test if osdep/asprintf.c is well built.
Just rename the function to asprintf2, detection will fail and you can test if everything still works properly.
Done. It works. But still… :)
Note 2: maybe there is some kind of helper to simplify the test, and still not in use for the strsep code, but I didn't find one. If there is, I'll of course update the code.
--- a/configure +++ b/configure @@ -3811,6 +3811,19 @@ fi
+echocheck "asprintf()" +_asprintf=no +statement_check stdio.h 'char *s; asprintf(&s, "%s:%d", "hello", 0x2a)' && _asprintf=yes +if test "$_asprintf" = yes ; then
Please use just "asprintf" as variable name, I plan to eventually drop all leading underscores.
You just fixed the _need_* variables into need_* (for example there is still _strsep). I fixed locally the _need_asprintf. Should I do the same with _asprintf?
--- /dev/null +++ b/osdep/asprintf.c @@ -0,0 +1,50 @@ +/* + * asprintf implementation for non-gnu systems
nit: GNU
Fixed locally.
No comment about the implementation; I haven't even read the asprintf man page yet.
--- /dev/null +++ b/osdep/asprintf.h @@ -0,0 +1,26 @@ +/* + * asprintf implementation for non-gnu systems
ditto
Fixed locally. -- Clément B.
On Thu, Jan 27, 2011 at 09:11:25PM +0100, Clément Bœsch wrote:
Sorry for the late reply. What I would expect might happen is that in C99 mode asprintf is not available (and we certainly don't want to have yet another thing that requires _GNU_SOURCE everywhere), so configure detection will fail. However linking will fail as well, since then asprintf is defined both in the libc and in MPlayer.
I'm not sure I get the difference between the compilation test made at ./configure and the way mplayer is built; the asprintf test does not check if we have _GNU_SOURCE defined, it try to compile a source code using it. Or did I missed something again?
I realize it all doesn't make much sense, and I should just say: to get asprintf we need to define _GNU_SOURCE. But we absolutely should avoid this in any way possible. So even when we use asprintf we should do so via a wrapper, so that we have to define _GNU_SOURCE on in the one file where the wrapper is implemented. Just in case you are curious, my original thoughts were: Well, without _GNU_SOURCE it is not in the header. Ideally the test should fail (I just realize this probably is not the case) then since without a declaration it cannot be used safely, particularly on 64 bit systems. Thus we use our own implementation instead. That would be a bit silly in itself, since we would be using it always and never a system one. Either way, linking fails because that has the same name as a symbol in the libc. So we'd be stuck with defining _GNU_SOURCE everywhere where we used asprintf
On Thu, Jan 27, 2011 at 09:34:04PM +0100, Reimar Döffinger wrote:
On Thu, Jan 27, 2011 at 09:11:25PM +0100, Clément Bœsch wrote:
Sorry for the late reply. What I would expect might happen is that in C99 mode asprintf is not available (and we certainly don't want to have yet another thing that requires _GNU_SOURCE everywhere), so configure detection will fail. However linking will fail as well, since then asprintf is defined both in the libc and in MPlayer.
I'm not sure I get the difference between the compilation test made at ./configure and the way mplayer is built; the asprintf test does not check if we have _GNU_SOURCE defined, it try to compile a source code using it. Or did I missed something again?
I realize it all doesn't make much sense, and I should just say: to get asprintf we need to define _GNU_SOURCE. But we absolutely should avoid this in any way possible. So even when we use asprintf we should do so via a wrapper, so that we have to define _GNU_SOURCE on in the one file where the wrapper is implemented. Just in case you are curious, my original thoughts were: Well, without _GNU_SOURCE it is not in the header. Ideally the test should fail (I just realize this probably is not the case) then since without a declaration it cannot be used safely, particularly on 64 bit systems. Thus we use our own implementation instead. That would be a bit silly in itself, since we would be using it always and never a system one. Either way, linking fails because that has the same name as a symbol in the libc. So we'd be stuck with defining _GNU_SOURCE everywhere where we used asprintf
Ok then, so let's go back to the first implementation? Patch re-attached… -- Clément B.
On Tue, Feb 01, 2011 at 11:19:02PM +0100, Clément Bœsch wrote:
On Thu, Jan 27, 2011 at 09:34:04PM +0100, Reimar Döffinger wrote:
On Thu, Jan 27, 2011 at 09:11:25PM +0100, Clément Bœsch wrote:
Sorry for the late reply. What I would expect might happen is that in C99 mode asprintf is not available (and we certainly don't want to have yet another thing that requires _GNU_SOURCE everywhere), so configure detection will fail. However linking will fail as well, since then asprintf is defined both in the libc and in MPlayer.
I'm not sure I get the difference between the compilation test made at ./configure and the way mplayer is built; the asprintf test does not check if we have _GNU_SOURCE defined, it try to compile a source code using it. Or did I missed something again?
I realize it all doesn't make much sense, and I should just say: to get asprintf we need to define _GNU_SOURCE. But we absolutely should avoid this in any way possible. So even when we use asprintf we should do so via a wrapper, so that we have to define _GNU_SOURCE on in the one file where the wrapper is implemented. Just in case you are curious, my original thoughts were: Well, without _GNU_SOURCE it is not in the header. Ideally the test should fail (I just realize this probably is not the case) then since without a declaration it cannot be used safely, particularly on 64 bit systems. Thus we use our own implementation instead. That would be a bit silly in itself, since we would be using it always and never a system one. Either way, linking fails because that has the same name as a symbol in the libc. So we'd be stuck with defining _GNU_SOURCE everywhere where we used asprintf
Ok then, so let's go back to the first implementation? Patch re-attached…
[...]
From 64a5f85883b1cc513a969b735f9e3c6f03723659 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux@gmail.com> Date: Wed, 12 Jan 2011 23:37:11 +0100 Subject: [PATCH] Add mp_strings.c with mp_asprintf function.
Since no one can make a decision about this, I'll apply this last one in 3 days. I don't mind changing all over again the code, but just tell me what you think. Regards. -- Clément B.
On Sat, Feb 05, 2011 at 10:20:37PM +0100, Clément Bœsch wrote:
On Tue, Feb 01, 2011 at 11:19:02PM +0100, Clément Bœsch wrote:
On Thu, Jan 27, 2011 at 09:34:04PM +0100, Reimar Döffinger wrote:
On Thu, Jan 27, 2011 at 09:11:25PM +0100, Clément Bœsch wrote:
Sorry for the late reply. What I would expect might happen is that in C99 mode asprintf is not available (and we certainly don't want to have yet another thing that requires _GNU_SOURCE everywhere), so configure detection will fail. However linking will fail as well, since then asprintf is defined both in the libc and in MPlayer.
I'm not sure I get the difference between the compilation test made at ./configure and the way mplayer is built; the asprintf test does not check if we have _GNU_SOURCE defined, it try to compile a source code using it. Or did I missed something again?
I realize it all doesn't make much sense, and I should just say: to get asprintf we need to define _GNU_SOURCE. But we absolutely should avoid this in any way possible. So even when we use asprintf we should do so via a wrapper, so that we have to define _GNU_SOURCE on in the one file where the wrapper is implemented. Just in case you are curious, my original thoughts were: Well, without _GNU_SOURCE it is not in the header. Ideally the test should fail (I just realize this probably is not the case) then since without a declaration it cannot be used safely, particularly on 64 bit systems. Thus we use our own implementation instead. That would be a bit silly in itself, since we would be using it always and never a system one. Either way, linking fails because that has the same name as a symbol in the libc. So we'd be stuck with defining _GNU_SOURCE everywhere where we used asprintf
Ok then, so let's go back to the first implementation? Patch re-attached…
[...]
From 64a5f85883b1cc513a969b735f9e3c6f03723659 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux@gmail.com> Date: Wed, 12 Jan 2011 23:37:11 +0100 Subject: [PATCH] Add mp_strings.c with mp_asprintf function.
Since no one can make a decision about this, I'll apply this last one in 3 days. I don't mind changing all over again the code, but just tell me what you think.
It's fine by me in principle, except that I am still somewhat suspicious of the va_* stuff (that used to be quite tricky/buggy at some points in time) but if you think it really is useful I won't stop you.
On Sat, Feb 05, 2011 at 10:45:57PM +0100, Reimar Döffinger wrote:
[...] It's fine by me in principle, except that I am still somewhat suspicious of the va_* stuff (that used to be quite tricky/buggy at some points in time) but if you think it really is useful I won't stop you.
I think it could be really useful in various places. As an example I attached to this mail two patches which use it. Also re-attached mp_asprintf since I shamefully forgot a +1. If anyone thinks those examples are a valid reason to add mp_asprintf I'll commit it, and will wait a review for those two next patches. -- Clément B.
On Sat, Feb 19, 2011 at 02:53:27PM +0100, Clément Bœsch wrote:
@@ -164,14 +164,21 @@ check4proxies( URL_t *url ) { #endif
mp_msg(MSGT_NETWORK,MSGL_V,"Using HTTP proxy: %s\n", proxy_url->url ); - len = make_http_proxy_url(proxy_url, url->url, NULL, 0) + 1; - new_url = malloc(len); + + if (proxy_url->username) + mp_asprintf(&new_url, "http_proxy://%s:%s@%s:%d/%s", + proxy_url->username, + proxy_url->password ? proxy_url->password : "", + proxy_url->hostname, proxy_url->port, url->url); + else + mp_asprintf(&new_url, "http_proxy://%s:%d/%s", + proxy_url->hostname, proxy_url->port, url->url); + if( new_url==NULL ) { mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); url_free(proxy_url); return url_out; } - make_http_proxy_url(proxy_url, url->url, new_url, len); tmp_url = url_new( new_url );
Please keep the separate functions, it makes the code easier to read.
- snprintf(meta, sizeof(meta), "%.4s", (char *) &sh_video->format); + mp_asprintf(&meta, "%.4s", (char *) &sh_video->format); else - snprintf(meta, sizeof(meta), "0x%08X", sh_video->format); - return strdup(meta); + mp_asprintf(&meta, "0x%08X", sh_video->format); + return meta;
That would be a good bit nicer if the function just returned the pointer... Is there are reasonable chance we will ever want/need the int return value?
On Sat, Feb 19, 2011 at 04:40:31PM +0100, Reimar Döffinger wrote:
On Sat, Feb 19, 2011 at 02:53:27PM +0100, Clément Bœsch wrote:
@@ -164,14 +164,21 @@ check4proxies( URL_t *url ) { #endif
mp_msg(MSGT_NETWORK,MSGL_V,"Using HTTP proxy: %s\n", proxy_url->url ); - len = make_http_proxy_url(proxy_url, url->url, NULL, 0) + 1; - new_url = malloc(len); + + if (proxy_url->username) + mp_asprintf(&new_url, "http_proxy://%s:%s@%s:%d/%s", + proxy_url->username, + proxy_url->password ? proxy_url->password : "", + proxy_url->hostname, proxy_url->port, url->url); + else + mp_asprintf(&new_url, "http_proxy://%s:%d/%s", + proxy_url->hostname, proxy_url->port, url->url); + if( new_url==NULL ) { mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); url_free(proxy_url); return url_out; } - make_http_proxy_url(proxy_url, url->url, new_url, len); tmp_url = url_new( new_url );
Please keep the separate functions, it makes the code easier to read.
Ok, changed.
- snprintf(meta, sizeof(meta), "%.4s", (char *) &sh_video->format); + mp_asprintf(&meta, "%.4s", (char *) &sh_video->format); else - snprintf(meta, sizeof(meta), "0x%08X", sh_video->format); - return strdup(meta); + mp_asprintf(&meta, "0x%08X", sh_video->format); + return meta;
That would be a good bit nicer if the function just returned the pointer...
Sure, it was just to be consistent with GNU asprintf.
Is there are reasonable chance we will ever want/need the int return value?
I don't think so. Patches updated. -- Clément B.
On Thu, Feb 24, 2011 at 01:09:28AM +0100, Clément Bœsch wrote:
Patches updated.
Seems fine to me.
-static int make_noauth_url(URL_t *url, char *dst, int dst_size) +static char *get_noauth_url(URL_t *url) -int make_http_proxy_url(URL_t *proxy, const char *host_url, char *dst, - int dst_size) +char *get_http_proxy_url(URL_t *proxy, const char *host_url)
Since you're changing it anyway, I think you could change URL_t to const URL_t. Doesn't matter much though.
On Sat, Feb 26, 2011 at 12:20:04PM +0100, Reimar Döffinger wrote:
On Thu, Feb 24, 2011 at 01:09:28AM +0100, Clément Bœsch wrote:
Patches updated.
Seems fine to me.
Thanks, applied.
-static int make_noauth_url(URL_t *url, char *dst, int dst_size) +static char *get_noauth_url(URL_t *url) -int make_http_proxy_url(URL_t *proxy, const char *host_url, char *dst, - int dst_size) +char *get_http_proxy_url(URL_t *proxy, const char *host_url)
Since you're changing it anyway, I think you could change URL_t to const URL_t. Doesn't matter much though.
Applied in a different commit. -- Clément B.
participants (3)
-
Clément Bœsch -
Diego Biurrun -
Reimar Döffinger