Re: [Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
Chun Yan Liu writes (Re: [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file): On 6/29/2015 at 06:52 PM, in message 21905.9050.452111.208...@mariner.uk.xensource.com, Ian Jackson ian.jack...@eu.citrix.com wrote: Chun Yan Liu writes (Re: [PATCH V5 2/7] libxl_read_file_contents: add new But if we are intending to use this with sysfs files, where the reported size is known to be wrong, it seems to me that we should be more proactive. If only for sysfs files, the bigger size problem should never happens, since sysfs system is not like normal file system, user won't be able to change the size. Reference from following link: http://www.makelinux.net/books/lkd2/ch17lev1sec8 I don't think that can be relied on as a guide to the future API. Maybe sysfs will grow support for bigger files in the future. (Also, that is actually a description of the kernel internals, not of the syscall API). Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
On 6/30/2015 at 05:08 PM, in message 21906.23698.778991.627...@mariner.uk.xensource.com, Ian Jackson ian.jack...@eu.citrix.com wrote: Chun Yan Liu writes (Re: [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file): On 6/29/2015 at 06:52 PM, in message 21905.9050.452111.208...@mariner.uk.xensource.com, Ian Jackson ian.jack...@eu.citrix.com wrote: Chun Yan Liu writes (Re: [PATCH V5 2/7] libxl_read_file_contents: add new But if we are intending to use this with sysfs files, where the reported size is known to be wrong, it seems to me that we should be more proactive. If only for sysfs files, the bigger size problem should never happens, since sysfs system is not like normal file system, user won't be able to change the size. Reference from following link: http://www.makelinux.net/books/lkd2/ch17lev1sec8 I don't think that can be relied on as a guide to the future API. Maybe sysfs will grow support for bigger files in the future. (Also, that is actually a description of the kernel internals, not of the syscall API). Yes, that's about kernel internals. But syscall API will finally call kernel implementation. From those description, we knows why fstat always return size 4096 (on x86_64, although actual file content length is less). And it's not possible the file is changed into a bigger size during we are reading it. About whether it'll be changed in future, really don't know. As to adding a check, it's certainly OK. I can update. Thanks, Chunyan Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
Chun Yan Liu writes (Re: [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file): On 6/30/2015 at 05:08 PM, in message 21906.23698.778991.627...@mariner.uk.xensource.com, Ian Jackson ian.jack...@eu.citrix.com wrote: Maybe sysfs will grow support for bigger files in the future. (Also, that is actually a description of the kernel internals, not of the syscall API). Yes, that's about kernel internals. But syscall API will finally call kernel implementation. From those description, we knows why fstat always return size 4096 (on x86_64, although actual file content length is less). And it's not possible the file is changed into a bigger size during we are reading it. About whether it'll be changed in future, really don't know. Right. My point was that an internals document is a poor guide to future behaviour. So, As to adding a check, it's certainly OK. I can update. Yes, please. Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
On 6/26/2015 at 09:05 PM, in message
21901.20009.85407.581...@mariner.uk.xensource.com, Ian Jackson
ian.jack...@eu.citrix.com wrote:
Chun Yan Liu writes (Re: [PATCH V5 2/7] libxl_read_file_contents: add new
entry to read sysfs file):
On 6/25/2015 at 07:09 PM, in message
21899.57676.368102.982...@mariner.uk.xensource.com, Ian Jackson
ian.jack...@eu.citrix.com wrote:
Chunyan Liu writes ([PATCH V5 2/7] libxl_read_file_contents: add new
entry Add a new entry libxl_read_sysfs_file_contents to handle sysfs
file
specially. It would be used in later pvusb work.
I think this still fails to detect a situation where the file is
unexpectedly longer than the requested size ?
+} else if (feof(f)) {
+if (rs datalen tolerate_shrinking_file) {
+datalen = rs;
+} else {
If the file is bigger than the requested size, it will fall to this
branch and report error.
I don't think this is true. I just applied your patch to my copy of
staging to see what the code looks like and saw this:
if (stab.st_size data_r) {
data = malloc(datalen);
if (!data) goto xe;
rs = fread(data, 1, datalen, f);
if (rs != datalen) {
if (ferror(f)) {
LOGE(ERROR, failed to read %s, filename);
goto xe;
} else if (feof(f)) {
if (rs datalen tolerate_shrinking_file) {
datalen = rs;
} else {
LOG(ERROR, %s changed size while we were reading it,
filename);
goto xe;
}
} else {
abort();
}
}
}
So I think in the case of a sysfs file which is 4096 bytes:
- stat will succeed and give st_size == 4096
- fread(,,4096,) will read the first 4096 bytes and return 4096
- rs == datalen, so we don't take the `errors and special
cases' branch
- we return success having read 4096 bytes
But please feel free to explain why I'm wrong.
You are right. I'm confused about the original logic. The original code
doesn't consider this case (size bigger than requested) at all. So, we need
to add a check if (rs == datalen read end of file), if not, means bigger
than requested, report error.
- Chunyan
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
Chun Yan Liu writes (Re: [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file): On 6/26/2015 at 09:05 PM, in message 21901.20009.85407.581...@mariner.uk.xensource.com, Ian Jackson ian.jack...@eu.citrix.com wrote: But please feel free to explain why I'm wrong. You are right. I'm confused about the original logic. The original code doesn't consider this case (size bigger than requested) at all. Indeed. The original code assumes that files don't change size, and only detects them shrinking because that causes a short read which it has deal with somehow. But if we are intending to use this with sysfs files, where the reported size is known to be wrong, it seems to me that we should be more proactive. So, we need to add a check if (rs == datalen read end of file), if not, means bigger than requested, report error. To detect a growing file it will be necessary to actually attempt to read at least one byte more than expected. This is probably done most conveniently by making the buffer one byte bigger, too. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
On 6/29/2015 at 06:52 PM, in message 21905.9050.452111.208...@mariner.uk.xensource.com, Ian Jackson ian.jack...@eu.citrix.com wrote: Chun Yan Liu writes (Re: [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file): On 6/26/2015 at 09:05 PM, in message 21901.20009.85407.581...@mariner.uk.xensource.com, Ian Jackson ian.jack...@eu.citrix.com wrote: But please feel free to explain why I'm wrong. You are right. I'm confused about the original logic. The original code doesn't consider this case (size bigger than requested) at all. Indeed. The original code assumes that files don't change size, and only detects them shrinking because that causes a short read which it has deal with somehow. But if we are intending to use this with sysfs files, where the reported size is known to be wrong, it seems to me that we should be more proactive. If only for sysfs files, the bigger size problem should never happens, since sysfs system is not like normal file system, user won't be able to change the size. Reference from following link: http://www.makelinux.net/books/lkd2/ch17lev1sec8 [ The sysfs filesystem is an in-memory virtual filesystem that provides a view of the kobject object hierarchy. It enables users to view the device topology of their system as a simple filesystem. Using attributes, kobjects can export files that allow kernel variables to be read from and optionally written to. .. The show() method is invoked on read. It must copy the value of the attribute given by attr into the buffer provided by buffer. The buffer is PAGE_SIZE bytes in length; on x86, PAGE_SIZE is 4096 bytes. The function should return the size in bytes of data actually written into buffer on success or a negative error code on failure. The store() method is invoked on write. It must read the size bytes from buffer into the variable represented by the attribute attr. The size of the buffer is always PAGE_SIZE or smaller. The function should return the size in bytes of data read from buffer on success or a negative error code on failure.] - Chunyan So, we need to add a check if (rs == datalen read end of file), if not, means bigger than requested, report error. To detect a growing file it will be necessary to actually attempt to read at least one byte more than expected. This is probably done most conveniently by making the buffer one byte bigger, too. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
Chun Yan Liu writes (Re: [PATCH V5 2/7] libxl_read_file_contents: add new
entry to read sysfs file):
On 6/25/2015 at 07:09 PM, in message
21899.57676.368102.982...@mariner.uk.xensource.com, Ian Jackson
ian.jack...@eu.citrix.com wrote:
Chunyan Liu writes ([PATCH V5 2/7] libxl_read_file_contents: add new
entry Add a new entry libxl_read_sysfs_file_contents to handle sysfs
file
specially. It would be used in later pvusb work.
I think this still fails to detect a situation where the file is
unexpectedly longer than the requested size ?
+} else if (feof(f)) {
+if (rs datalen tolerate_shrinking_file) {
+datalen = rs;
+} else {
If the file is bigger than the requested size, it will fall to this
branch and report error.
I don't think this is true. I just applied your patch to my copy of
staging to see what the code looks like and saw this:
if (stab.st_size data_r) {
data = malloc(datalen);
if (!data) goto xe;
rs = fread(data, 1, datalen, f);
if (rs != datalen) {
if (ferror(f)) {
LOGE(ERROR, failed to read %s, filename);
goto xe;
} else if (feof(f)) {
if (rs datalen tolerate_shrinking_file) {
datalen = rs;
} else {
LOG(ERROR, %s changed size while we were reading it,
filename);
goto xe;
}
} else {
abort();
}
}
}
So I think in the case of a sysfs file which is 4096 bytes:
- stat will succeed and give st_size == 4096
- fread(,,4096,) will read the first 4096 bytes and return 4096
- rs == datalen, so we don't take the `errors and special
cases' branch
- we return success having read 4096 bytes
But please feel free to explain why I'm wrong.
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
On 6/25/2015 at 07:09 PM, in message
21899.57676.368102.982...@mariner.uk.xensource.com, Ian Jackson
ian.jack...@eu.citrix.com wrote:
Chunyan Liu writes ([PATCH V5 2/7] libxl_read_file_contents: add new entry
to read sysfs file):
Sysfs file has size=4096 but actual file content is less than that.
Current libxl_read_file_contents will treat it as error when file size
and actual file content differs, so reading sysfs file content with
this function always fails.
Add a new entry libxl_read_sysfs_file_contents to handle sysfs file
specially. It would be used in later pvusb work.
I think this still fails to detect a situation where the file is
unexpectedly longer than the requested size ?
+} else if (feof(f)) {
+if (rs datalen tolerate_shrinking_file) {
+datalen = rs;
+} else {
If the file is bigger than the requested size, it will fall to this branch and
report error.
Do you mean I should report another error message separately?
- Chunyan
+LOG(ERROR, %s changed size while we were reading it,
+filename);
+goto xe;
+}
+} else {
As we wrote earlier:
Is there any risk that the file is actually bigger than advertised,
rather than smaller ?
For sysfs file, couldn't be bigger.
Then you should detect the condition that the file is bigger, and call
it an error.
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
On 6/25/2015 at 07:09 PM, in message
21899.57676.368102.982...@mariner.uk.xensource.com, Ian Jackson
ian.jack...@eu.citrix.com wrote:
Chunyan Liu writes ([PATCH V5 2/7] libxl_read_file_contents: add new entry
to read sysfs file):
Sysfs file has size=4096 but actual file content is less than that.
Current libxl_read_file_contents will treat it as error when file size
and actual file content differs, so reading sysfs file content with
this function always fails.
Add a new entry libxl_read_sysfs_file_contents to handle sysfs file
specially. It would be used in later pvusb work.
I think this still fails to detect a situation where the file is
unexpectedly longer than the requested size ?
+} else if (feof(f)) {
+if (rs datalen tolerate_shrinking_file) {
+datalen = rs;
+} else {
If the file is bigger than the requested size, it will fall to this branch and
report error.
Do you mean I should report another error message separately?
- Chunyan
+LOG(ERROR, %s changed size while we were reading it,
+filename);
+goto xe;
+}
+} else {
As we wrote earlier:
Is there any risk that the file is actually bigger than advertised,
rather than smaller ?
For sysfs file, couldn't be bigger.
Then you should detect the condition that the file is bigger, and call
it an error.
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
Chunyan Liu writes ([PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file): Sysfs file has size=4096 but actual file content is less than that. Current libxl_read_file_contents will treat it as error when file size and actual file content differs, so reading sysfs file content with this function always fails. Add a new entry libxl_read_sysfs_file_contents to handle sysfs file specially. It would be used in later pvusb work. I think this still fails to detect a situation where the file is unexpectedly longer than the requested size ? As we wrote earlier: Is there any risk that the file is actually bigger than advertised, rather than smaller ? For sysfs file, couldn't be bigger. Then you should detect the condition that the file is bigger, and call it an error. Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH V5 2/7] libxl_read_file_contents: add new entry to read sysfs file
Sysfs file has size=4096 but actual file content is less than that.
Current libxl_read_file_contents will treat it as error when file size
and actual file content differs, so reading sysfs file content with
this function always fails.
Add a new entry libxl_read_sysfs_file_contents to handle sysfs file
specially. It would be used in later pvusb work.
Signed-off-by: Chunyan Liu cy...@suse.com
---
Changes:
- make libxl_read_file_contents as internal.
- handle tolerant_shriking_file as Ian suggested
tools/libxl/libxl_internal.h | 2 ++
tools/libxl/libxl_utils.c| 37 +
2 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index cdbe28b..0965e08 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3707,6 +3707,8 @@ static inline void libxl__update_config_vtpm(libxl__gc
*gc,
void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
const libxl_bitmap *sptr);
#endif
+_hidden int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char
*filename,
+ void **data_r, int *datalen_r);
/*
* Local variables:
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index f6be2d7..dc5465e 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -322,8 +322,10 @@ out:
return rc;
}
-int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
- void **data_r, int *datalen_r) {
+static int libxl_read_file_contents_core(libxl_ctx *ctx, const char *filename,
+ void **data_r, int *datalen_r,
+ bool tolerate_shrinking_file)
+{
GC_INIT(ctx);
FILE *f = 0;
uint8_t *data = 0;
@@ -364,14 +366,20 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char
*filename,
rs = fread(data, 1, datalen, f);
if (rs != datalen) {
-if (ferror(f))
+if (ferror(f)) {
LOGE(ERROR, failed to read %s, filename);
-else if (feof(f))
-LOG(ERROR, %s changed size while we were reading it,
- filename);
-else
+goto xe;
+} else if (feof(f)) {
+if (rs datalen tolerate_shrinking_file) {
+datalen = rs;
+} else {
+LOG(ERROR, %s changed size while we were reading it,
+filename);
+goto xe;
+}
+} else {
abort();
-goto xe;
+}
}
}
@@ -396,6 +404,19 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char
*filename,
return e;
}
+int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
+ void **data_r, int *datalen_r)
+{
+return libxl_read_file_contents_core(ctx, filename, data_r, datalen_r, 0);
+}
+
+int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
+ void **data_r, int *datalen_r)
+{
+return libxl_read_file_contents_core(ctx, filename, data_r, datalen_r, 1);
+}
+
+
#define READ_WRITE_EXACTLY(rw, zero_is_eof, constdata)\
\
int libxl_##rw##_exactly(libxl_ctx *ctx, int fd, \
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
