diff mbox

[FFmpeg-devel,v2,2/2] avformat/utils: simplify the ff_mkdir_p with SEPARATOR

Message ID 20191201140042.26305-2-lance.lmwang@gmail.com
State Superseded
Headers show

Commit Message

Lance Wang Dec. 1, 2019, 2 p.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/utils.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Hendrik Leppkes Dec. 1, 2019, 4:33 p.m. UTC | #1
On Sun, Dec 1, 2019 at 3:08 PM <lance.lmwang@gmail.com> wrote:
>
> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/utils.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 579e6d6..993e6d2 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -4843,12 +4843,17 @@ void av_url_split(char *proto, int proto_size,
>      }
>  }
>
> +#if HAVE_DOS_PATHS
> +#define SEPARATOR '\\'
> +#else
> +#define SEPARATOR '/'
> +#endif
> +
>  int ff_mkdir_p(const char *path)
>  {
>      int ret = 0;
>      char *temp = av_strdup(path);
>      char *pos = temp;
> -    char tmp_ch = '\0';
>
>      if (!path || !temp) {
>          return -1;
> @@ -4856,19 +4861,18 @@ int ff_mkdir_p(const char *path)
>
>      if (*temp == '.')
>          pos++;
> -    if (*temp == '/' || *temp == '\\')
> +    if (*temp == SEPARATOR)
>          pos++;
>
>      for ( ; *pos != '\0'; ++pos) {
> -        if (*pos == '/' || *pos == '\\') {
> -            tmp_ch = *pos;
> +        if (*pos == SEPARATOR) {
>              *pos = '\0';
>              ret = mkdir(temp, 0755);
> -            *pos = tmp_ch;
> +            *pos = SEPARATOR;
>          }
>      }
>
> -    if ((*(pos - 1) != '/') || (*(pos - 1) != '\\')) {
> +    if (*(pos - 1) != SEPARATOR) {
>          ret = mkdir(temp, 0755);
>      }

I think there is some value to be able to specify a path with both
kinds of slashes. For example, most of everything else on Windows will
accept normal slashes, in addition to the default backslash.

- Hendrik
Lance Wang Dec. 2, 2019, 2:16 a.m. UTC | #2
On Sun, Dec 01, 2019 at 05:33:16PM +0100, Hendrik Leppkes wrote:
> On Sun, Dec 1, 2019 at 3:08 PM <lance.lmwang@gmail.com> wrote:
> >
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/utils.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 579e6d6..993e6d2 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -4843,12 +4843,17 @@ void av_url_split(char *proto, int proto_size,
> >      }
> >  }
> >
> > +#if HAVE_DOS_PATHS
> > +#define SEPARATOR '\\'
> > +#else
> > +#define SEPARATOR '/'
> > +#endif
> > +
> >  int ff_mkdir_p(const char *path)
> >  {
> >      int ret = 0;
> >      char *temp = av_strdup(path);
> >      char *pos = temp;
> > -    char tmp_ch = '\0';
> >
> >      if (!path || !temp) {
> >          return -1;
> > @@ -4856,19 +4861,18 @@ int ff_mkdir_p(const char *path)
> >
> >      if (*temp == '.')
> >          pos++;
> > -    if (*temp == '/' || *temp == '\\')
> > +    if (*temp == SEPARATOR)
> >          pos++;
> >
> >      for ( ; *pos != '\0'; ++pos) {
> > -        if (*pos == '/' || *pos == '\\') {
> > -            tmp_ch = *pos;
> > +        if (*pos == SEPARATOR) {
> >              *pos = '\0';
> >              ret = mkdir(temp, 0755);
> > -            *pos = tmp_ch;
> > +            *pos = SEPARATOR;
> >          }
> >      }
> >
> > -    if ((*(pos - 1) != '/') || (*(pos - 1) != '\\')) {
> > +    if (*(pos - 1) != SEPARATOR) {
> >          ret = mkdir(temp, 0755);
> >      }
> 
> I think there is some value to be able to specify a path with both
> kinds of slashes. For example, most of everything else on Windows will
> accept normal slashes, in addition to the default backslash.
Hendrik, I haven't got your point yet, can you make it clear so that I
can change the patch for your case.

For example, on Linux, if the path is:
 ~/Movies/hl\\s/vs%v/manifest.m3u8

The current code will mkdir below path:
path: /Users
path: /Users/lmwang
path: /Users/lmwang/Movies
path: /Users/lmwang/Movies/hl   >>> unexpected
path: /Users/lmwang/Movies/hl\s
path: /Users/lmwang/Movies/hl\s/vs0

You can see /Users/lmwang/Movies/hl directory isn't expected directory which is created.

After applied the patch, it'll not create it anymore.

> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Hendrik Leppkes Dec. 2, 2019, 8:26 a.m. UTC | #3
On Mon, Dec 2, 2019 at 3:16 AM Limin Wang <lance.lmwang@gmail.com> wrote:
>
> On Sun, Dec 01, 2019 at 05:33:16PM +0100, Hendrik Leppkes wrote:
> > On Sun, Dec 1, 2019 at 3:08 PM <lance.lmwang@gmail.com> wrote:
> > >
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > >
> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > ---
> > >  libavformat/utils.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > index 579e6d6..993e6d2 100644
> > > --- a/libavformat/utils.c
> > > +++ b/libavformat/utils.c
> > > @@ -4843,12 +4843,17 @@ void av_url_split(char *proto, int proto_size,
> > >      }
> > >  }
> > >
> > > +#if HAVE_DOS_PATHS
> > > +#define SEPARATOR '\\'
> > > +#else
> > > +#define SEPARATOR '/'
> > > +#endif
> > > +
> > >  int ff_mkdir_p(const char *path)
> > >  {
> > >      int ret = 0;
> > >      char *temp = av_strdup(path);
> > >      char *pos = temp;
> > > -    char tmp_ch = '\0';
> > >
> > >      if (!path || !temp) {
> > >          return -1;
> > > @@ -4856,19 +4861,18 @@ int ff_mkdir_p(const char *path)
> > >
> > >      if (*temp == '.')
> > >          pos++;
> > > -    if (*temp == '/' || *temp == '\\')
> > > +    if (*temp == SEPARATOR)
> > >          pos++;
> > >
> > >      for ( ; *pos != '\0'; ++pos) {
> > > -        if (*pos == '/' || *pos == '\\') {
> > > -            tmp_ch = *pos;
> > > +        if (*pos == SEPARATOR) {
> > >              *pos = '\0';
> > >              ret = mkdir(temp, 0755);
> > > -            *pos = tmp_ch;
> > > +            *pos = SEPARATOR;
> > >          }
> > >      }
> > >
> > > -    if ((*(pos - 1) != '/') || (*(pos - 1) != '\\')) {
> > > +    if (*(pos - 1) != SEPARATOR) {
> > >          ret = mkdir(temp, 0755);
> > >      }
> >
> > I think there is some value to be able to specify a path with both
> > kinds of slashes. For example, most of everything else on Windows will
> > accept normal slashes, in addition to the default backslash.
> Hendrik, I haven't got your point yet, can you make it clear so that I
> can change the patch for your case.
>
> For example, on Linux, if the path is:
>  ~/Movies/hl\\s/vs%v/manifest.m3u8
>
> The current code will mkdir below path:
> path: /Users
> path: /Users/lmwang
> path: /Users/lmwang/Movies
> path: /Users/lmwang/Movies/hl   >>> unexpected
> path: /Users/lmwang/Movies/hl\s
> path: /Users/lmwang/Movies/hl\s/vs0
>
> You can see /Users/lmwang/Movies/hl directory isn't expected directory which is created.
>
> After applied the patch, it'll not create it anymore.
>

But if I'm on Windows and I specify a path with normal slashes (where
SEPARATOR  is backslash), I would absolutely expect it to create it
like that. Which it does right now.

- Hendrik
Lance Wang Dec. 2, 2019, 2:35 p.m. UTC | #4
On Mon, Dec 02, 2019 at 09:26:11AM +0100, Hendrik Leppkes wrote:
> On Mon, Dec 2, 2019 at 3:16 AM Limin Wang <lance.lmwang@gmail.com> wrote:
> >
> > On Sun, Dec 01, 2019 at 05:33:16PM +0100, Hendrik Leppkes wrote:
> > > On Sun, Dec 1, 2019 at 3:08 PM <lance.lmwang@gmail.com> wrote:
> > > >
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > >
> > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > ---
> > > >  libavformat/utils.c | 16 ++++++++++------
> > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > > index 579e6d6..993e6d2 100644
> > > > --- a/libavformat/utils.c
> > > > +++ b/libavformat/utils.c
> > > > @@ -4843,12 +4843,17 @@ void av_url_split(char *proto, int proto_size,
> > > >      }
> > > >  }
> > > >
> > > > +#if HAVE_DOS_PATHS
> > > > +#define SEPARATOR '\\'
> > > > +#else
> > > > +#define SEPARATOR '/'
> > > > +#endif
> > > > +
> > > >  int ff_mkdir_p(const char *path)
> > > >  {
> > > >      int ret = 0;
> > > >      char *temp = av_strdup(path);
> > > >      char *pos = temp;
> > > > -    char tmp_ch = '\0';
> > > >
> > > >      if (!path || !temp) {
> > > >          return -1;
> > > > @@ -4856,19 +4861,18 @@ int ff_mkdir_p(const char *path)
> > > >
> > > >      if (*temp == '.')
> > > >          pos++;
> > > > -    if (*temp == '/' || *temp == '\\')
> > > > +    if (*temp == SEPARATOR)
> > > >          pos++;
> > > >
> > > >      for ( ; *pos != '\0'; ++pos) {
> > > > -        if (*pos == '/' || *pos == '\\') {
> > > > -            tmp_ch = *pos;
> > > > +        if (*pos == SEPARATOR) {
> > > >              *pos = '\0';
> > > >              ret = mkdir(temp, 0755);
> > > > -            *pos = tmp_ch;
> > > > +            *pos = SEPARATOR;
> > > >          }
> > > >      }
> > > >
> > > > -    if ((*(pos - 1) != '/') || (*(pos - 1) != '\\')) {
> > > > +    if (*(pos - 1) != SEPARATOR) {
> > > >          ret = mkdir(temp, 0755);
> > > >      }
> > >
> > > I think there is some value to be able to specify a path with both
> > > kinds of slashes. For example, most of everything else on Windows will
> > > accept normal slashes, in addition to the default backslash.
> > Hendrik, I haven't got your point yet, can you make it clear so that I
> > can change the patch for your case.
> >
> > For example, on Linux, if the path is:
> >  ~/Movies/hl\\s/vs%v/manifest.m3u8
> >
> > The current code will mkdir below path:
> > path: /Users
> > path: /Users/lmwang
> > path: /Users/lmwang/Movies
> > path: /Users/lmwang/Movies/hl   >>> unexpected
> > path: /Users/lmwang/Movies/hl\s
> > path: /Users/lmwang/Movies/hl\s/vs0
> >
> > You can see /Users/lmwang/Movies/hl directory isn't expected directory which is created.
> >
> > After applied the patch, it'll not create it anymore.
> >
> 
> But if I'm on Windows and I specify a path with normal slashes (where
> SEPARATOR  is backslash), I would absolutely expect it to create it
> like that. Which it does right now.

Now I haven't windows system in hand, so I can't test your condition.
For windows, the old code will consider normal slashes(/) as path separator 
instead of special charactor. Maybe I have misunderstanding for that.

> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Hendrik Leppkes Dec. 2, 2019, 3:25 p.m. UTC | #5
On Mon, Dec 2, 2019 at 3:36 PM Limin Wang <lance.lmwang@gmail.com> wrote:
>
> On Mon, Dec 02, 2019 at 09:26:11AM +0100, Hendrik Leppkes wrote:
> > On Mon, Dec 2, 2019 at 3:16 AM Limin Wang <lance.lmwang@gmail.com> wrote:
> > >
> > > On Sun, Dec 01, 2019 at 05:33:16PM +0100, Hendrik Leppkes wrote:
> > > > On Sun, Dec 1, 2019 at 3:08 PM <lance.lmwang@gmail.com> wrote:
> > > > >
> > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > >
> > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > ---
> > > > >  libavformat/utils.c | 16 ++++++++++------
> > > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > > > index 579e6d6..993e6d2 100644
> > > > > --- a/libavformat/utils.c
> > > > > +++ b/libavformat/utils.c
> > > > > @@ -4843,12 +4843,17 @@ void av_url_split(char *proto, int proto_size,
> > > > >      }
> > > > >  }
> > > > >
> > > > > +#if HAVE_DOS_PATHS
> > > > > +#define SEPARATOR '\\'
> > > > > +#else
> > > > > +#define SEPARATOR '/'
> > > > > +#endif
> > > > > +
> > > > >  int ff_mkdir_p(const char *path)
> > > > >  {
> > > > >      int ret = 0;
> > > > >      char *temp = av_strdup(path);
> > > > >      char *pos = temp;
> > > > > -    char tmp_ch = '\0';
> > > > >
> > > > >      if (!path || !temp) {
> > > > >          return -1;
> > > > > @@ -4856,19 +4861,18 @@ int ff_mkdir_p(const char *path)
> > > > >
> > > > >      if (*temp == '.')
> > > > >          pos++;
> > > > > -    if (*temp == '/' || *temp == '\\')
> > > > > +    if (*temp == SEPARATOR)
> > > > >          pos++;
> > > > >
> > > > >      for ( ; *pos != '\0'; ++pos) {
> > > > > -        if (*pos == '/' || *pos == '\\') {
> > > > > -            tmp_ch = *pos;
> > > > > +        if (*pos == SEPARATOR) {
> > > > >              *pos = '\0';
> > > > >              ret = mkdir(temp, 0755);
> > > > > -            *pos = tmp_ch;
> > > > > +            *pos = SEPARATOR;
> > > > >          }
> > > > >      }
> > > > >
> > > > > -    if ((*(pos - 1) != '/') || (*(pos - 1) != '\\')) {
> > > > > +    if (*(pos - 1) != SEPARATOR) {
> > > > >          ret = mkdir(temp, 0755);
> > > > >      }
> > > >
> > > > I think there is some value to be able to specify a path with both
> > > > kinds of slashes. For example, most of everything else on Windows will
> > > > accept normal slashes, in addition to the default backslash.
> > > Hendrik, I haven't got your point yet, can you make it clear so that I
> > > can change the patch for your case.
> > >
> > > For example, on Linux, if the path is:
> > >  ~/Movies/hl\\s/vs%v/manifest.m3u8
> > >
> > > The current code will mkdir below path:
> > > path: /Users
> > > path: /Users/lmwang
> > > path: /Users/lmwang/Movies
> > > path: /Users/lmwang/Movies/hl   >>> unexpected
> > > path: /Users/lmwang/Movies/hl\s
> > > path: /Users/lmwang/Movies/hl\s/vs0
> > >
> > > You can see /Users/lmwang/Movies/hl directory isn't expected directory which is created.
> > >
> > > After applied the patch, it'll not create it anymore.
> > >
> >
> > But if I'm on Windows and I specify a path with normal slashes (where
> > SEPARATOR  is backslash), I would absolutely expect it to create it
> > like that. Which it does right now.
>
> Now I haven't windows system in hand, so I can't test your condition.
> For windows, the old code will consider normal slashes(/) as path separator
> instead of special charactor. Maybe I have misunderstanding for that.
>

Yes, it does, and it should continue to do so, because everything else
on Windows does as well. Both slash and backslash are valid path
seperators.

- Hendrik
Lance Wang Dec. 3, 2019, 2:20 p.m. UTC | #6
On Mon, Dec 02, 2019 at 04:25:14PM +0100, Hendrik Leppkes wrote:
> On Mon, Dec 2, 2019 at 3:36 PM Limin Wang <lance.lmwang@gmail.com> wrote:
> >
> > On Mon, Dec 02, 2019 at 09:26:11AM +0100, Hendrik Leppkes wrote:
> > > On Mon, Dec 2, 2019 at 3:16 AM Limin Wang <lance.lmwang@gmail.com> wrote:
> > > >
> > > > On Sun, Dec 01, 2019 at 05:33:16PM +0100, Hendrik Leppkes wrote:
> > > > > On Sun, Dec 1, 2019 at 3:08 PM <lance.lmwang@gmail.com> wrote:
> > > > > >
> > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > >
> > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > > ---
> > > > > >  libavformat/utils.c | 16 ++++++++++------
> > > > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > > > > index 579e6d6..993e6d2 100644
> > > > > > --- a/libavformat/utils.c
> > > > > > +++ b/libavformat/utils.c
> > > > > > @@ -4843,12 +4843,17 @@ void av_url_split(char *proto, int proto_size,
> > > > > >      }
> > > > > >  }
> > > > > >
> > > > > > +#if HAVE_DOS_PATHS
> > > > > > +#define SEPARATOR '\\'
> > > > > > +#else
> > > > > > +#define SEPARATOR '/'
> > > > > > +#endif
> > > > > > +
> > > > > >  int ff_mkdir_p(const char *path)
> > > > > >  {
> > > > > >      int ret = 0;
> > > > > >      char *temp = av_strdup(path);
> > > > > >      char *pos = temp;
> > > > > > -    char tmp_ch = '\0';
> > > > > >
> > > > > >      if (!path || !temp) {
> > > > > >          return -1;
> > > > > > @@ -4856,19 +4861,18 @@ int ff_mkdir_p(const char *path)
> > > > > >
> > > > > >      if (*temp == '.')
> > > > > >          pos++;
> > > > > > -    if (*temp == '/' || *temp == '\\')
> > > > > > +    if (*temp == SEPARATOR)
> > > > > >          pos++;
> > > > > >
> > > > > >      for ( ; *pos != '\0'; ++pos) {
> > > > > > -        if (*pos == '/' || *pos == '\\') {
> > > > > > -            tmp_ch = *pos;
> > > > > > +        if (*pos == SEPARATOR) {
> > > > > >              *pos = '\0';
> > > > > >              ret = mkdir(temp, 0755);
> > > > > > -            *pos = tmp_ch;
> > > > > > +            *pos = SEPARATOR;
> > > > > >          }
> > > > > >      }
> > > > > >
> > > > > > -    if ((*(pos - 1) != '/') || (*(pos - 1) != '\\')) {
> > > > > > +    if (*(pos - 1) != SEPARATOR) {
> > > > > >          ret = mkdir(temp, 0755);
> > > > > >      }
> > > > >
> > > > > I think there is some value to be able to specify a path with both
> > > > > kinds of slashes. For example, most of everything else on Windows will
> > > > > accept normal slashes, in addition to the default backslash.
> > > > Hendrik, I haven't got your point yet, can you make it clear so that I
> > > > can change the patch for your case.
> > > >
> > > > For example, on Linux, if the path is:
> > > >  ~/Movies/hl\\s/vs%v/manifest.m3u8
> > > >
> > > > The current code will mkdir below path:
> > > > path: /Users
> > > > path: /Users/lmwang
> > > > path: /Users/lmwang/Movies
> > > > path: /Users/lmwang/Movies/hl   >>> unexpected
> > > > path: /Users/lmwang/Movies/hl\s
> > > > path: /Users/lmwang/Movies/hl\s/vs0
> > > >
> > > > You can see /Users/lmwang/Movies/hl directory isn't expected directory which is created.
> > > >
> > > > After applied the patch, it'll not create it anymore.
> > > >
> > >
> > > But if I'm on Windows and I specify a path with normal slashes (where
> > > SEPARATOR  is backslash), I would absolutely expect it to create it
> > > like that. Which it does right now.
> >
> > Now I haven't windows system in hand, so I can't test your condition.
> > For windows, the old code will consider normal slashes(/) as path separator
> > instead of special charactor. Maybe I have misunderstanding for that.
> >
> 
> Yes, it does, and it should continue to do so, because everything else
> on Windows does as well. Both slash and backslash are valid path
> seperators.

Sorry, I don't know slash is valid path seperator on windows. I'll try
to cross build a window version to test how to support this case.

> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox

Patch

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 579e6d6..993e6d2 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -4843,12 +4843,17 @@  void av_url_split(char *proto, int proto_size,
     }
 }
 
+#if HAVE_DOS_PATHS
+#define SEPARATOR '\\'
+#else
+#define SEPARATOR '/'
+#endif
+
 int ff_mkdir_p(const char *path)
 {
     int ret = 0;
     char *temp = av_strdup(path);
     char *pos = temp;
-    char tmp_ch = '\0';
 
     if (!path || !temp) {
         return -1;
@@ -4856,19 +4861,18 @@  int ff_mkdir_p(const char *path)
 
     if (*temp == '.')
         pos++;
-    if (*temp == '/' || *temp == '\\')
+    if (*temp == SEPARATOR)
         pos++;
 
     for ( ; *pos != '\0'; ++pos) {
-        if (*pos == '/' || *pos == '\\') {
-            tmp_ch = *pos;
+        if (*pos == SEPARATOR) {
             *pos = '\0';
             ret = mkdir(temp, 0755);
-            *pos = tmp_ch;
+            *pos = SEPARATOR;
         }
     }
 
-    if ((*(pos - 1) != '/') || (*(pos - 1) != '\\')) {
+    if (*(pos - 1) != SEPARATOR) {
         ret = mkdir(temp, 0755);
     }