test-container: Use copy_file_range_compat for cross-device copy [BZ #23597]
Commit Message
On Thu, Aug 30, 2018 at 12:10 PM, DJ Delorie <dj@redhat.com> wrote:
>
> Ah, thanks for figuring that out. I'll add a fallback unless someone
> else feels like doing it before I do ;-)
Something like this?
Comments
On 30/08/2018 18:26, H.J. Lu wrote:
> On Thu, Aug 30, 2018 at 12:10 PM, DJ Delorie <dj@redhat.com> wrote:
>> Ah, thanks for figuring that out. I'll add a fallback unless someone
>> else feels like doing it before I do ;-)
> Something like this?
>
>
> -- H.J.
>
>
> 0001-test-container-Use-copy_file_range_compat-for-cross-.patch
>
>
> From c6b06cc42ed58d6c8f9cdd7e21e89214ce7b2a18 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Thu, 30 Aug 2018 14:21:14 -0700
> Subject: [PATCH] test-container: Use copy_file_range_compat for cross-device
> copy [BZ #23597]
>
> copy_file_range can't be used to copy a file from glibc source directory
> to glibc build directory since they may be on different filesystems.
> This uses copy_file_range_compat for cross-device copy.
>
> [BZ #23597]
> * io/copy_file_range-compat.c (COPY_FILE_RANGE): Allow
> cross-device copies for test-container.c.
> * support/test-container.c (COPY_FILE_RANGE_DECL): New.
> (COPY_FILE_RANGE): Likewise.
> (__libc_lseek64): Likewise.
> Include <io/copy_file_range-compat.c>.
> (copy_one_file): Call copy_file_range_compat for cross-device
> copy.
> ---
> io/copy_file_range-compat.c | 3 +++
> support/test-container.c | 9 ++++++++-
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/io/copy_file_range-compat.c b/io/copy_file_range-compat.c
> index 4ab22cad19..15860a7480 100644
> --- a/io/copy_file_range-compat.c
> +++ b/io/copy_file_range-compat.c
> @@ -59,12 +59,15 @@ COPY_FILE_RANGE (int infd, __off64_t *pinoff,
> __set_errno (EINVAL);
> return -1;
> }
> +#ifndef SUPPORT_TEST_DRIVER_H
> + /* Allow cross-device copies for test-container.c. */
> if (instat.st_dev != outstat.st_dev)
> {
> /* Cross-device copies are not supported. */
> __set_errno (EXDEV);
> return -1;
> }
> +#endif
I not very found of tying libsupport implementation with libc code,
even more by supplying an internal symbol with subtle different
semantics than the libc one.
We don't actually need all the internal error checks or support of
copy_file_range, wouldn't be easier to just pread/pwrite the file
contents with a much more simpler implementation?
> }
>
> /* The output descriptor must not have O_APPEND set. */
> diff --git a/support/test-container.c b/support/test-container.c
> index 2e91bdf9ec..ee99af2d55 100644
> --- a/support/test-container.c
> +++ b/support/test-container.c
> @@ -47,6 +47,12 @@
> #include "check.h"
> #include "test-driver.h"
>
> +/* Compile a local version of copy_file_range. */
> +#define COPY_FILE_RANGE_DECL static
> +#define COPY_FILE_RANGE copy_file_range_compat
> +#define __libc_lseek64 lseek64
> +#include <io/copy_file_range-compat.c>
> +
> #ifndef __linux__
> #define mount(s,t,fs,f,d) no_mount()
> int no_mount (void)
> @@ -383,7 +389,8 @@ copy_one_file (const char *sname, const char *dname)
> if (dfd < 0)
> FAIL_EXIT1 ("unable to open %s for writing\n", dname);
>
> - if (copy_file_range (sfd, 0, dfd, 0, st.st_size, 0) != st.st_size)
> + /* Use copy_file_range_compat for cross-device copy. */
> + if (copy_file_range_compat (sfd, 0, dfd, 0, st.st_size, 0) != st.st_size)
> FAIL_EXIT1 ("cannot copy file %s to %s\n", sname, dname);
>
> xclose (sfd);
> -- 2.17.1
>
On Fri, Aug 31, 2018 at 5:02 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
> On 30/08/2018 18:26, H.J. Lu wrote:
>> On Thu, Aug 30, 2018 at 12:10 PM, DJ Delorie <dj@redhat.com> wrote:
>>> Ah, thanks for figuring that out. I'll add a fallback unless someone
>>> else feels like doing it before I do ;-)
>> Something like this?
>>
>>
>> -- H.J.
>>
>>
>> 0001-test-container-Use-copy_file_range_compat-for-cross-.patch
>>
>>
>> From c6b06cc42ed58d6c8f9cdd7e21e89214ce7b2a18 Mon Sep 17 00:00:00 2001
>> From: "H.J. Lu" <hjl.tools@gmail.com>
>> Date: Thu, 30 Aug 2018 14:21:14 -0700
>> Subject: [PATCH] test-container: Use copy_file_range_compat for cross-device
>> copy [BZ #23597]
>>
>> copy_file_range can't be used to copy a file from glibc source directory
>> to glibc build directory since they may be on different filesystems.
>> This uses copy_file_range_compat for cross-device copy.
>>
>> [BZ #23597]
>> * io/copy_file_range-compat.c (COPY_FILE_RANGE): Allow
>> cross-device copies for test-container.c.
>> * support/test-container.c (COPY_FILE_RANGE_DECL): New.
>> (COPY_FILE_RANGE): Likewise.
>> (__libc_lseek64): Likewise.
>> Include <io/copy_file_range-compat.c>.
>> (copy_one_file): Call copy_file_range_compat for cross-device
>> copy.
>> ---
>> io/copy_file_range-compat.c | 3 +++
>> support/test-container.c | 9 ++++++++-
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/io/copy_file_range-compat.c b/io/copy_file_range-compat.c
>> index 4ab22cad19..15860a7480 100644
>> --- a/io/copy_file_range-compat.c
>> +++ b/io/copy_file_range-compat.c
>> @@ -59,12 +59,15 @@ COPY_FILE_RANGE (int infd, __off64_t *pinoff,
>> __set_errno (EINVAL);
>> return -1;
>> }
>> +#ifndef SUPPORT_TEST_DRIVER_H
>> + /* Allow cross-device copies for test-container.c. */
>> if (instat.st_dev != outstat.st_dev)
>> {
>> /* Cross-device copies are not supported. */
>> __set_errno (EXDEV);
>> return -1;
>> }
>> +#endif
>
> I not very found of tying libsupport implementation with libc code,
> even more by supplying an internal symbol with subtle different
> semantics than the libc one.
>
> We don't actually need all the internal error checks or support of
> copy_file_range, wouldn't be easier to just pread/pwrite the file
> contents with a much more simpler implementation?
That is what io/copy_file_range-compat.c does. Are you suggesting
cut and paste, instead of #include?
On 08/31/2018 10:40 AM, H.J. Lu wrote:
> On Fri, Aug 31, 2018 at 5:02 AM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>> On 30/08/2018 18:26, H.J. Lu wrote:
>>> On Thu, Aug 30, 2018 at 12:10 PM, DJ Delorie <dj@redhat.com> wrote:
>>>> Ah, thanks for figuring that out. I'll add a fallback unless someone
>>>> else feels like doing it before I do ;-)
>>> Something like this?
>>>
>>>
>>> -- H.J.
>>>
>>>
>>> 0001-test-container-Use-copy_file_range_compat-for-cross-.patch
>>>
>>>
>>> From c6b06cc42ed58d6c8f9cdd7e21e89214ce7b2a18 Mon Sep 17 00:00:00 2001
>>> From: "H.J. Lu" <hjl.tools@gmail.com>
>>> Date: Thu, 30 Aug 2018 14:21:14 -0700
>>> Subject: [PATCH] test-container: Use copy_file_range_compat for cross-device
>>> copy [BZ #23597]
>>>
>>> copy_file_range can't be used to copy a file from glibc source directory
>>> to glibc build directory since they may be on different filesystems.
>>> This uses copy_file_range_compat for cross-device copy.
>>>
>>> [BZ #23597]
>>> * io/copy_file_range-compat.c (COPY_FILE_RANGE): Allow
>>> cross-device copies for test-container.c.
>>> * support/test-container.c (COPY_FILE_RANGE_DECL): New.
>>> (COPY_FILE_RANGE): Likewise.
>>> (__libc_lseek64): Likewise.
>>> Include <io/copy_file_range-compat.c>.
>>> (copy_one_file): Call copy_file_range_compat for cross-device
>>> copy.
>>> ---
>>> io/copy_file_range-compat.c | 3 +++
>>> support/test-container.c | 9 ++++++++-
>>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/io/copy_file_range-compat.c b/io/copy_file_range-compat.c
>>> index 4ab22cad19..15860a7480 100644
>>> --- a/io/copy_file_range-compat.c
>>> +++ b/io/copy_file_range-compat.c
>>> @@ -59,12 +59,15 @@ COPY_FILE_RANGE (int infd, __off64_t *pinoff,
>>> __set_errno (EINVAL);
>>> return -1;
>>> }
>>> +#ifndef SUPPORT_TEST_DRIVER_H
>>> + /* Allow cross-device copies for test-container.c. */
>>> if (instat.st_dev != outstat.st_dev)
>>> {
>>> /* Cross-device copies are not supported. */
>>> __set_errno (EXDEV);
>>> return -1;
>>> }
>>> +#endif
>>
>> I not very found of tying libsupport implementation with libc code,
>> even more by supplying an internal symbol with subtle different
>> semantics than the libc one.
>>
>> We don't actually need all the internal error checks or support of
>> copy_file_range, wouldn't be easier to just pread/pwrite the file
>> contents with a much more simpler implementation?
>
> That is what io/copy_file_range-compat.c does. Are you suggesting
> cut and paste, instead of #include?
Not quite.
We are suggesting a unique and simplified copy of those sources to
be placed into the support/ directory as a *.c file that can be
used for support purposes.
Coupling support behaviour and runtime behaviour is a bad choice,
so we should not do it. Even if it means some code duplication.
From c6b06cc42ed58d6c8f9cdd7e21e89214ce7b2a18 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 30 Aug 2018 14:21:14 -0700
Subject: [PATCH] test-container: Use copy_file_range_compat for cross-device
copy [BZ #23597]
copy_file_range can't be used to copy a file from glibc source directory
to glibc build directory since they may be on different filesystems.
This uses copy_file_range_compat for cross-device copy.
[BZ #23597]
* io/copy_file_range-compat.c (COPY_FILE_RANGE): Allow
cross-device copies for test-container.c.
* support/test-container.c (COPY_FILE_RANGE_DECL): New.
(COPY_FILE_RANGE): Likewise.
(__libc_lseek64): Likewise.
Include <io/copy_file_range-compat.c>.
(copy_one_file): Call copy_file_range_compat for cross-device
copy.
---
io/copy_file_range-compat.c | 3 +++
support/test-container.c | 9 ++++++++-
2 files changed, 11 insertions(+), 1 deletion(-)
@@ -59,12 +59,15 @@ COPY_FILE_RANGE (int infd, __off64_t *pinoff,
__set_errno (EINVAL);
return -1;
}
+#ifndef SUPPORT_TEST_DRIVER_H
+ /* Allow cross-device copies for test-container.c. */
if (instat.st_dev != outstat.st_dev)
{
/* Cross-device copies are not supported. */
__set_errno (EXDEV);
return -1;
}
+#endif
}
/* The output descriptor must not have O_APPEND set. */
@@ -47,6 +47,12 @@
#include "check.h"
#include "test-driver.h"
+/* Compile a local version of copy_file_range. */
+#define COPY_FILE_RANGE_DECL static
+#define COPY_FILE_RANGE copy_file_range_compat
+#define __libc_lseek64 lseek64
+#include <io/copy_file_range-compat.c>
+
#ifndef __linux__
#define mount(s,t,fs,f,d) no_mount()
int no_mount (void)
@@ -383,7 +389,8 @@ copy_one_file (const char *sname, const char *dname)
if (dfd < 0)
FAIL_EXIT1 ("unable to open %s for writing\n", dname);
- if (copy_file_range (sfd, 0, dfd, 0, st.st_size, 0) != st.st_size)
+ /* Use copy_file_range_compat for cross-device copy. */
+ if (copy_file_range_compat (sfd, 0, dfd, 0, st.st_size, 0) != st.st_size)
FAIL_EXIT1 ("cannot copy file %s to %s\n", sname, dname);
xclose (sfd);
--
2.17.1