Discussion:
[vdr] vdr 1.7.23: patch for handling symlinks in recordings directory as earlier
(too old to reply)
sundararaj reel
2012-01-17 13:26:48 UTC
Permalink
Hi,

I am attaching a patch for vdr 1.7.23 for the problem described here:
http://www.vdr-portal.de/board1-news/board2-vdr-news/p1047199-announce-vdr-developer-version-1-7-23/#post1047199

There appears to be a problem in listing recordings due to a bug fix
in vdr 1.7.23. "Fixed handling symbolic links in
cRecordings::ScanVideoDir()"

The attached patch just disables the translation of symbolic links to
"real" paths. So that all recordings appear to be under the same
(recordings) directory tree, as it was earlier.

Please reply with your results.
--
Thanks,
Sundararaj
Klaus Schmidinger
2012-01-25 09:29:16 UTC
Permalink
Post by sundararaj reel
Hi,
http://www.vdr-portal.de/board1-news/board2-vdr-news/p1047199-announce-vdr-developer-version-1-7-23/#post1047199
There appears to be a problem in listing recordings due to a bug fix
in vdr 1.7.23. "Fixed handling symbolic links in
cRecordings::ScanVideoDir()"
The attached patch just disables the translation of symbolic links to
"real" paths. So that all recordings appear to be under the same
(recordings) directory tree, as it was earlier.
Please reply with your results.
Can somebody who has actually this use case please confirm
whether this patch fixes the problem?

Klaus
Oliver Endriss
2012-01-25 13:11:48 UTC
Permalink
Post by Klaus Schmidinger
Post by sundararaj reel
Hi,
http://www.vdr-portal.de/board1-news/board2-vdr-news/p1047199-announce-vdr-developer-version-1-7-23/#post1047199
There appears to be a problem in listing recordings due to a bug fix
in vdr 1.7.23. "Fixed handling symbolic links in
cRecordings::ScanVideoDir()"
The attached patch just disables the translation of symbolic links to
"real" paths. So that all recordings appear to be under the same
(recordings) directory tree, as it was earlier.
Please reply with your results.
Can somebody who has actually this use case please confirm
whether this patch fixes the problem?
Confirmed.

Without this patch, symbolic links are not displayed
correctly on my machine.

Oliver
--
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/
Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/
----------------------------------------------------------------
Klaus Schmidinger
2012-01-26 10:07:18 UTC
Permalink
Post by Oliver Endriss
Post by Klaus Schmidinger
Post by sundararaj reel
Hi,
http://www.vdr-portal.de/board1-news/board2-vdr-news/p1047199-announce-vdr-developer-version-1-7-23/#post1047199
There appears to be a problem in listing recordings due to a bug fix
in vdr 1.7.23. "Fixed handling symbolic links in
cRecordings::ScanVideoDir()"
The attached patch just disables the translation of symbolic links to
"real" paths. So that all recordings appear to be under the same
(recordings) directory tree, as it was earlier.
Please reply with your results.
Can somebody who has actually this use case please confirm
whether this patch fixes the problem?
Confirmed.
Without this patch, symbolic links are not displayed
correctly on my machine.
Oliver
Thanks.

I believe the second call to stat() is now superfluous.
Can you please confirm that the following patch still works
as expected?

--- recording.c 2012/01/25 09:32:39 2.45
+++ recording.c 2012/01/26 10:02:29
@@ -1120,11 +1120,6 @@
continue;
}
Link = 1;
- buffer = ReadLink(buffer);
- if (!*buffer)
- continue;
- if (stat(buffer, &st) != 0)
- continue;
}
if (S_ISDIR(st.st_mode)) {
if (endswith(buffer, deleted ? DELEXT : RECEXT)) {


Klaus
Oliver Endriss
2012-01-27 12:04:11 UTC
Permalink
Post by Klaus Schmidinger
Post by Oliver Endriss
Post by Klaus Schmidinger
Post by sundararaj reel
Hi,
http://www.vdr-portal.de/board1-news/board2-vdr-news/p1047199-announce-vdr-developer-version-1-7-23/#post1047199
There appears to be a problem in listing recordings due to a bug fix
in vdr 1.7.23. "Fixed handling symbolic links in
cRecordings::ScanVideoDir()"
The attached patch just disables the translation of symbolic links to
"real" paths. So that all recordings appear to be under the same
(recordings) directory tree, as it was earlier.
Please reply with your results.
Can somebody who has actually this use case please confirm
whether this patch fixes the problem?
Confirmed.
Without this patch, symbolic links are not displayed
correctly on my machine.
Oliver
Thanks.
I believe the second call to stat() is now superfluous.
Can you please confirm that the following patch still works
as expected?
--- recording.c 2012/01/25 09:32:39 2.45
+++ recording.c 2012/01/26 10:02:29
@@ -1120,11 +1120,6 @@
continue;
}
Link = 1;
- buffer = ReadLink(buffer);
- if (!*buffer)
- continue;
- if (stat(buffer, &st) != 0)
- continue;
}
if (S_ISDIR(st.st_mode)) {
if (endswith(buffer, deleted ? DELEXT : RECEXT)) {
Yes, it does not make any difference here.

CU
Oliver
--
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/
Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/
----------------------------------------------------------------
Lucian Muresan
2012-02-12 22:11:17 UTC
Permalink
Post by Oliver Endriss
Post by Klaus Schmidinger
Post by Oliver Endriss
Post by Klaus Schmidinger
Post by sundararaj reel
Hi,
http://www.vdr-portal.de/board1-news/board2-vdr-news/p1047199-announce-vdr-developer-version-1-7-23/#post1047199
There appears to be a problem in listing recordings due to a bug fix
in vdr 1.7.23. "Fixed handling symbolic links in
cRecordings::ScanVideoDir()"
The attached patch just disables the translation of symbolic links to
"real" paths. So that all recordings appear to be under the same
(recordings) directory tree, as it was earlier.
Please reply with your results.
Can somebody who has actually this use case please confirm
whether this patch fixes the problem?
Confirmed.
Without this patch, symbolic links are not displayed
correctly on my machine.
Oliver
Thanks.
I believe the second call to stat() is now superfluous.
Can you please confirm that the following patch still works
as expected?
--- recording.c 2012/01/25 09:32:39 2.45
+++ recording.c 2012/01/26 10:02:29
@@ -1120,11 +1120,6 @@
continue;
}sundararaj reel
Link = 1;
- buffer = ReadLink(buffer);
- if (!*buffer)
- continue;
- if (stat(buffer,&st) != 0)
- continue;
}
if (S_ISDIR(st.st_mode)) {
if (endswith(buffer, deleted ? DELEXT : RECEXT)) {
Yes, it does not make any difference here.
Well, with this patch, symbolic links are not displayed at all on my VDR
machine, whereas with sundararaj reel's fix they are displayed correctly.

Cheers,
Lucian
Klaus Schmidinger
2012-02-13 09:43:51 UTC
Permalink
Post by Oliver Endriss
Post by Klaus Schmidinger
Post by Oliver Endriss
Post by Klaus Schmidinger
Post by sundararaj reel
Hi,
http://www.vdr-portal.de/board1-news/board2-vdr-news/p1047199-announce-vdr-developer-version-1-7-23/#post1047199
There appears to be a problem in listing recordings due to a bug fix
in vdr 1.7.23. "Fixed handling symbolic links in
cRecordings::ScanVideoDir()"
The attached patch just disables the translation of symbolic links to
"real" paths. So that all recordings appear to be under the same
(recordings) directory tree, as it was earlier.
Please reply with your results.
Can somebody who has actually this use case please confirm
whether this patch fixes the problem?
Confirmed.
Without this patch, symbolic links are not displayed
correctly on my machine.
Oliver
Thanks.
I believe the second call to stat() is now superfluous.
Can you please confirm that the following patch still works
as expected?
--- recording.c 2012/01/25 09:32:39 2.45
+++ recording.c 2012/01/26 10:02:29
@@ -1120,11 +1120,6 @@
continue;
}sundararaj reel
How did this "sundararaj reel" get in here?
Post by Oliver Endriss
Post by Klaus Schmidinger
Link = 1;
- buffer = ReadLink(buffer);
- if (!*buffer)
- continue;
- if (stat(buffer,&st) != 0)
- continue;
}
if (S_ISDIR(st.st_mode)) {
if (endswith(buffer, deleted ? DELEXT : RECEXT)) {
Yes, it does not make any difference here.
Well, with this patch, symbolic links are not displayed at all on my VDR machine, whereas with sundararaj reel's fix they are displayed correctly.
So what you're saying it that this...

----------------------------------------------------------------------------------------------
--- a/recording.c
+++ b/recording.c
@@ -1120,9 +1120,13 @@ void cRecordings::ScanVideoDir(const char *DirName, bool Foreground, int LinkLev
continue;
}
Link = 1;
+#if 0
+ // do not resolve the symbolic links in paths to real path
+ // thereby keeping all the recordings under one directory
buffer = ReadLink(buffer);
if (!*buffer)
continue;
+#endif
if (stat(buffer, &st) != 0)
continue;
}
----------------------------------------------------------------------------------------------

...works, while this...

----------------------------------------------------------------------------------------------
--- recording.c 2012/01/25 09:32:39 2.45
+++ recording.c 2012/01/26 10:02:29
@@ -1120,11 +1120,6 @@
continue;
}
Link = 1;
- buffer = ReadLink(buffer);
- if (!*buffer)
- continue;
- if (stat(buffer, &st) != 0)
- continue;
}
if (S_ISDIR(st.st_mode)) {
if (endswith(buffer, deleted ? DELEXT : RECEXT)) {
----------------------------------------------------------------------------------------------

...doesn't?

I find that hard to believe, because the only difference here is that
the second version removes the stat() call, which is superfluous if
'buffer' is no longer modified.

Klaus
Lucian Muresan
2012-02-13 10:44:46 UTC
Permalink
[...]
Post by Klaus Schmidinger
Post by Lucian Muresan
Well, with this patch, symbolic links are not displayed at all on my
VDR machine, whereas with sundararaj reel's fix they are displayed
correctly.
So what you're saying it that this...
----------------------------------------------------------------------------------------------
--- a/recording.c
+++ b/recording.c
@@ -1120,9 +1120,13 @@ void cRecordings::ScanVideoDir(const char
*DirName, bool Foreground, int LinkLev
continue;
}
Link = 1;
+#if 0
+ // do not resolve the symbolic links in paths to real path
+ // thereby keeping all the recordings under one directory
buffer = ReadLink(buffer);
if (!*buffer)
continue;
+#endif
if (stat(buffer, &st) != 0)
continue;
}
----------------------------------------------------------------------------------------------
...works, while this...
----------------------------------------------------------------------------------------------
--- recording.c 2012/01/25 09:32:39 2.45
+++ recording.c 2012/01/26 10:02:29
@@ -1120,11 +1120,6 @@
continue;
}
Link = 1;
- buffer = ReadLink(buffer);
- if (!*buffer)
- continue;
- if (stat(buffer, &st) != 0)
- continue;
}
if (S_ISDIR(st.st_mode)) {
if (endswith(buffer, deleted ? DELEXT : RECEXT)) {
----------------------------------------------------------------------------------------------
...doesn't?
I find that hard to believe, because the only difference here is that
the second version removes the stat() call, which is superfluous if
'buffer' is no longer modified.
Haven't really looked at the code, until now, and I also do not exactly
know what the call to stat does and also didn't try to understand the
whole picture now, but your patch does not simply remove just the call
to stat, but also a *continue* statement from the *while* loop, this
could have strong implications, so just please consider analyzing the
issue with respect to that.

Regards,
Lucian
Klaus Schmidinger
2012-02-13 11:00:22 UTC
Permalink
[...]
Post by Klaus Schmidinger
Post by Lucian Muresan
Well, with this patch, symbolic links are not displayed at all on my
VDR machine, whereas with sundararaj reel's fix they are displayed
correctly.
So what you're saying it that this...
----------------------------------------------------------------------------------------------
--- a/recording.c
+++ b/recording.c
@@ -1120,9 +1120,13 @@ void cRecordings::ScanVideoDir(const char
*DirName, bool Foreground, int LinkLev
continue;
}
Link = 1;
+#if 0
+ // do not resolve the symbolic links in paths to real path
+ // thereby keeping all the recordings under one directory
buffer = ReadLink(buffer);
if (!*buffer)
continue;
+#endif
if (stat(buffer, &st) != 0)
continue;
}
----------------------------------------------------------------------------------------------
...works, while this...
----------------------------------------------------------------------------------------------
--- recording.c 2012/01/25 09:32:39 2.45
+++ recording.c 2012/01/26 10:02:29
@@ -1120,11 +1120,6 @@
continue;
}
Link = 1;
- buffer = ReadLink(buffer);
- if (!*buffer)
- continue;
- if (stat(buffer, &st) != 0)
- continue;
}
if (S_ISDIR(st.st_mode)) {
if (endswith(buffer, deleted ? DELEXT : RECEXT)) {
----------------------------------------------------------------------------------------------
...doesn't?
I find that hard to believe, because the only difference here is that
the second version removes the stat() call, which is superfluous if
'buffer' is no longer modified.
Haven't really looked at the code, until now, and I also do not exactly know what the call to stat does and also didn't try to understand the whole picture now, but your patch does not simply remove just the call to stat, but also a *continue* statement from the *while* loop, this could have strong
implications, so just please consider analyzing the issue with respect to that.
The original code was

----------------------------------------------------------------------------------------------
if (stat(buffer, &st) == 0) {
int Link = 0;
if (S_ISLNK(st.st_mode)) {
if (LinkLevel > MAX_LINK_LEVEL) {
isyslog("max link level exceeded - not scanning %s", *buffer);
continue;
}
Link = 1;
buffer = ReadLink(buffer);
if (!*buffer)
continue;
if (stat(buffer, &st) != 0)
continue;
}
...
}
----------------------------------------------------------------------------------------------

After Sundararaj's patch it looked like this (just leaving out the lines
that his '#if 0' disabled):

----------------------------------------------------------------------------------------------
if (stat(buffer, &st) == 0) {
int Link = 0;
if (S_ISLNK(st.st_mode)) {
if (LinkLevel > MAX_LINK_LEVEL) {
isyslog("max link level exceeded - not scanning %s", *buffer);
continue;
}
if (stat(buffer, &st) != 0)
continue;
}
...
}
----------------------------------------------------------------------------------------------

Reducing this to the stat() calls results in

----------------------------------------------------------------------------------------------
if (stat(buffer, &st) == 0) {
...
if (stat(buffer, &st) != 0)
continue;
}
...
}
----------------------------------------------------------------------------------------------

As you can see, 'buffer' is no longer modified between the two calls, so they
will both return the same value. The code sequence is only entered if the first
stat() call returns 0, so the second call will also return 0, and thus the
'continue' statement will never be executed.

Klaus
Lucian Muresan
2012-02-13 11:27:14 UTC
Permalink
Post by Klaus Schmidinger
Post by Lucian Muresan
[...]
Post by Klaus Schmidinger
Post by Lucian Muresan
Well, with this patch, symbolic links are not displayed at all on my
VDR machine, whereas with sundararaj reel's fix they are displayed
correctly.
So what you're saying it that this...
----------------------------------------------------------------------------------------------
--- a/recording.c
+++ b/recording.c
@@ -1120,9 +1120,13 @@ void cRecordings::ScanVideoDir(const char
*DirName, bool Foreground, int LinkLev
continue;
}
Link = 1;
+#if 0
+ // do not resolve the symbolic links in paths to real path
+ // thereby keeping all the recordings under one directory
buffer = ReadLink(buffer);
if (!*buffer)
continue;
+#endif
if (stat(buffer, &st) != 0)
continue;
}
----------------------------------------------------------------------------------------------
...works, while this...
----------------------------------------------------------------------------------------------
--- recording.c 2012/01/25 09:32:39 2.45
+++ recording.c 2012/01/26 10:02:29
@@ -1120,11 +1120,6 @@
continue;
}
Link = 1;
- buffer = ReadLink(buffer);
- if (!*buffer)
- continue;
- if (stat(buffer, &st) != 0)
- continue;
}
if (S_ISDIR(st.st_mode)) {
if (endswith(buffer, deleted ? DELEXT : RECEXT)) {
----------------------------------------------------------------------------------------------
...doesn't?
I find that hard to believe, because the only difference here is that
the second version removes the stat() call, which is superfluous if
'buffer' is no longer modified.
Haven't really looked at the code, until now, and I also do not
exactly know what the call to stat does and also didn't try to
understand the whole picture now, but your patch does not simply
remove just the call to stat, but also a *continue* statement from the
*while* loop, this could have strong
implications, so just please consider analyzing the issue with respect to that.
The original code was
----------------------------------------------------------------------------------------------
if (stat(buffer, &st) == 0) {
int Link = 0;
if (S_ISLNK(st.st_mode)) {
if (LinkLevel > MAX_LINK_LEVEL) {
isyslog("max link level exceeded - not scanning %s", *buffer);
continue;
}
Link = 1;
buffer = ReadLink(buffer);
if (!*buffer)
continue;
if (stat(buffer, &st) != 0)
continue;
}
...
}
----------------------------------------------------------------------------------------------
After Sundararaj's patch it looked like this (just leaving out the lines
----------------------------------------------------------------------------------------------
if (stat(buffer, &st) == 0) {
int Link = 0;
if (S_ISLNK(st.st_mode)) {
if (LinkLevel > MAX_LINK_LEVEL) {
isyslog("max link level exceeded - not scanning %s", *buffer);
continue;
}
if (stat(buffer, &st) != 0)
continue;
}
...
}
----------------------------------------------------------------------------------------------
Reducing this to the stat() calls results in
----------------------------------------------------------------------------------------------
if (stat(buffer, &st) == 0) {
...
if (stat(buffer, &st) != 0)
continue;
}
...
}
----------------------------------------------------------------------------------------------
As you can see, 'buffer' is no longer modified between the two calls, so they
will both return the same value. The code sequence is only entered if the first
stat() call returns 0, so the second call will also return 0, and thus the
'continue' statement will never be executed.
Now I looked a bit closer, and noticed that the "first" call to stat is
actually not to *stat*, but to *lstat* in vanilla vdr-1.7.23, so if you
remove the call to the second one, could it be that you're not really
cutting redundancy, maybe it actually makes a difference?

Lucian
Klaus Schmidinger
2012-02-13 11:54:05 UTC
Permalink
Post by Klaus Schmidinger
Post by Lucian Muresan
[...]
Post by Klaus Schmidinger
Post by Lucian Muresan
Well, with this patch, symbolic links are not displayed at all on my
VDR machine, whereas with sundararaj reel's fix they are displayed
correctly.
So what you're saying it that this...
----------------------------------------------------------------------------------------------
--- a/recording.c
+++ b/recording.c
@@ -1120,9 +1120,13 @@ void cRecordings::ScanVideoDir(const char
*DirName, bool Foreground, int LinkLev
continue;
}
Link = 1;
+#if 0
+ // do not resolve the symbolic links in paths to real path
+ // thereby keeping all the recordings under one directory
buffer = ReadLink(buffer);
if (!*buffer)
continue;
+#endif
if (stat(buffer, &st) != 0)
continue;
}
----------------------------------------------------------------------------------------------
...works, while this...
----------------------------------------------------------------------------------------------
--- recording.c 2012/01/25 09:32:39 2.45
+++ recording.c 2012/01/26 10:02:29
@@ -1120,11 +1120,6 @@
continue;
}
Link = 1;
- buffer = ReadLink(buffer);
- if (!*buffer)
- continue;
- if (stat(buffer, &st) != 0)
- continue;
}
if (S_ISDIR(st.st_mode)) {
if (endswith(buffer, deleted ? DELEXT : RECEXT)) {
----------------------------------------------------------------------------------------------
...doesn't?
I find that hard to believe, because the only difference here is that
the second version removes the stat() call, which is superfluous if
'buffer' is no longer modified.
Haven't really looked at the code, until now, and I also do not
exactly know what the call to stat does and also didn't try to
understand the whole picture now, but your patch does not simply
remove just the call to stat, but also a *continue* statement from the
*while* loop, this could have strong
implications, so just please consider analyzing the issue with respect to that.
The original code was
----------------------------------------------------------------------------------------------
if (stat(buffer, &st) == 0) {
int Link = 0;
if (S_ISLNK(st.st_mode)) {
if (LinkLevel > MAX_LINK_LEVEL) {
isyslog("max link level exceeded - not scanning %s", *buffer);
continue;
}
Link = 1;
buffer = ReadLink(buffer);
if (!*buffer)
continue;
if (stat(buffer, &st) != 0)
continue;
}
...
}
----------------------------------------------------------------------------------------------
After Sundararaj's patch it looked like this (just leaving out the lines
----------------------------------------------------------------------------------------------
if (stat(buffer, &st) == 0) {
int Link = 0;
if (S_ISLNK(st.st_mode)) {
if (LinkLevel > MAX_LINK_LEVEL) {
isyslog("max link level exceeded - not scanning %s", *buffer);
continue;
}
if (stat(buffer, &st) != 0)
continue;
}
...
}
----------------------------------------------------------------------------------------------
Reducing this to the stat() calls results in
----------------------------------------------------------------------------------------------
if (stat(buffer, &st) == 0) {
...
if (stat(buffer, &st) != 0)
continue;
}
...
}
----------------------------------------------------------------------------------------------
As you can see, 'buffer' is no longer modified between the two calls, so they
will both return the same value. The code sequence is only entered if the first
stat() call returns 0, so the second call will also return 0, and thus the
'continue' statement will never be executed.
Now I looked a bit closer, and noticed that the "first" call to stat is actually not to *stat*, but to *lstat* in vanilla vdr-1.7.23, so if you remove the call to the second one, could it be that you're not really cutting redundancy, maybe it actually makes a difference?
Now you got me ;-)
I had switched back to 'stat()' instead of 'lstat()' and forgotten to reintroduce
the 'lstat()'. Therefore I saw two identical calls to stat().
With the first call being lstat(), the second call to stat() is of course necessary
for the following S_ISDIR() check.

Sorry for the confusion, and thanks for clarifying this.
So now this whole code sequence looks like this:

----------------------------------------------------------------------------------------------
if (lstat(buffer, &st) == 0) {
int Link = 0;
if (S_ISLNK(st.st_mode)) {
if (LinkLevel > MAX_LINK_LEVEL) {
isyslog("max link level exceeded - not scanning %s", *buffer);
continue;
}
Link = 1;
if (stat(buffer, &st) != 0)
continue;
}
----------------------------------------------------------------------------------------------

Klaus
Lucian Muresan
2012-02-13 12:52:25 UTC
Permalink
[...]
Post by Klaus Schmidinger
Post by Lucian Muresan
Now I looked a bit closer, and noticed that the "first" call to stat
is actually not to *stat*, but to *lstat* in vanilla vdr-1.7.23, so if
you remove the call to the second one, could it be that you're not
really cutting redundancy, maybe it actually makes a difference?
Now you got me ;-)
I had switched back to 'stat()' instead of 'lstat()' and forgotten to reintroduce
the 'lstat()'. Therefore I saw two identical calls to stat().
With the first call being lstat(), the second call to stat() is of course necessary
for the following S_ISDIR() check.
Sorry for the confusion, and thanks for clarifying this.
----------------------------------------------------------------------------------------------
if (lstat(buffer, &st) == 0) {
int Link = 0;
if (S_ISLNK(st.st_mode)) {
if (LinkLevel > MAX_LINK_LEVEL) {
isyslog("max link level exceeded - not scanning %s", *buffer);
continue;
}
Link = 1;
if (stat(buffer, &st) != 0)
continue;
}
No problem at all, I'm glad it's now sorted out for the next developer
version.

Lucian

Loading...