Re: [Xen-devel] [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file
On Tue, Aug 11, 2015 at 08:37:09PM -0600, Chun Yan Liu wrote:
[...]
> > > +
> > > +if (rs < datalen) {
> > > +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 (tolerate_shrinking_file) {
> > > +datalen = rs;
> > > +} else {
> > > +LOG(ERROR, "%s shrunk size while we were reading
> > > it",
> > > +filename);
> > > +goto xe;
> > > +}
> > > +} else {
> > > abort();
> > > -goto xe;
> > > +}
> >
> > This is a bit bikeshedding, but you can leave "goto xe" out of two `if'
> > to reduce patch size.
>
> I guess you mean if (ferror(f)) and if (feof(f)) ? We can't leave 'goto xe'
> outside,
> since in if (feof(f)) && if (tolerate_shrinking_file), it's not error but an
> expected
> result in sysfs case.
>
Oh, right. I missed that tolerate_shrinking_file check. Sorry for the
noise.
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file
>>> On 8/11/2015 at 07:26 PM, in message
<2015082655.ge7...@zion.uk.xensource.com>, Wei Liu
wrote:
> On Mon, Aug 10, 2015 at 06:35:23PM +0800, Chunyan Liu wrote:
> > 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
> >
> > ---
> > Changes:
> > - read one more byte to check bigger size problem.
> >
> > tools/libxl/libxl_internal.h | 2 ++
> > tools/libxl/libxl_utils.c| 51
> > ++--
> > 2 files changed, 42 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 6013628..f98f089 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -4001,6 +4001,8 @@ void libxl__bitmap_copy_best_effort(libxl__gc *gc,
> libxl_bitmap *dptr,
> >
> > int libxl__count_physical_sockets(libxl__gc *gc, int *sockets);
> > #endif
> > +_hidden int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char
> *filename,
> > + void **data_r, int *datalen_r);
>
> Indentation looks wrong.
>
> >
> > /*
> > * Local variables:
> > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> > index bfc9699..9234efb 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;
> > @@ -359,20 +361,34 @@ int libxl_read_file_contents(libxl_ctx *ctx, const
> char *filename,
> > datalen = stab.st_size;
> >
> > if (stab.st_size && data_r) {
> > -data = malloc(datalen);
> > +data = malloc(datalen + 1);
> > if (!data) goto xe;
> >
> > -rs = fread(data, 1, datalen, f);
> > -if (rs != datalen) {
> > -if (ferror(f))
> > +rs = fread(data, 1, datalen + 1, f);
> > +if (rs > datalen) {
> > +LOG(ERROR, "%s increased size while we were reading it",
> > +filename);
> > +goto xe;
> > +}
> > +
> > +if (rs < datalen) {
> > +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 (tolerate_shrinking_file) {
> > +datalen = rs;
> > +} else {
> > +LOG(ERROR, "%s shrunk size while we were reading it",
> > +filename);
> > +goto xe;
> > +}
> > +} else {
> > abort();
> > -goto xe;
> > +}
>
> This is a bit bikeshedding, but you can leave "goto xe" out of two `if'
> to reduce patch size.
I guess you mean if (ferror(f)) and if (feof(f)) ? We can't leave 'goto xe'
outside,
since in if (feof(f)) && if (tolerate_shrinking_file), it's not error but an
expected
result in sysfs case.
> > }
> > +
> > +data = realloc(data, datalen);
>
> Should check return value of realloc.
Will add a check:
if (!data) goto xe;
Thanks,
Chunyan
>
> The logic of this function reflects what has been discussed so far.
>
> Wei.
>
> > }
> >
> > if (fclose(f)) {
> > @@ -396,6 +412,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,
Re: [Xen-devel] [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file
On Mon, Aug 10, 2015 at 06:35:23PM +0800, Chunyan Liu wrote:
> 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
>
> ---
> Changes:
> - read one more byte to check bigger size problem.
>
> tools/libxl/libxl_internal.h | 2 ++
> tools/libxl/libxl_utils.c| 51
> ++--
> 2 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6013628..f98f089 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -4001,6 +4001,8 @@ void libxl__bitmap_copy_best_effort(libxl__gc *gc,
> libxl_bitmap *dptr,
>
> int libxl__count_physical_sockets(libxl__gc *gc, int *sockets);
> #endif
> +_hidden int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char
> *filename,
> + void **data_r, int *datalen_r);
Indentation looks wrong.
>
> /*
> * Local variables:
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index bfc9699..9234efb 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;
> @@ -359,20 +361,34 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char
> *filename,
> datalen = stab.st_size;
>
> if (stab.st_size && data_r) {
> -data = malloc(datalen);
> +data = malloc(datalen + 1);
> if (!data) goto xe;
>
> -rs = fread(data, 1, datalen, f);
> -if (rs != datalen) {
> -if (ferror(f))
> +rs = fread(data, 1, datalen + 1, f);
> +if (rs > datalen) {
> +LOG(ERROR, "%s increased size while we were reading it",
> +filename);
> +goto xe;
> +}
> +
> +if (rs < datalen) {
> +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 (tolerate_shrinking_file) {
> +datalen = rs;
> +} else {
> +LOG(ERROR, "%s shrunk size while we were reading it",
> +filename);
> +goto xe;
> +}
> +} else {
> abort();
> -goto xe;
> +}
This is a bit bikeshedding, but you can leave "goto xe" out of two `if'
to reduce patch size.
> }
> +
> +data = realloc(data, datalen);
Should check return value of realloc.
The logic of this function reflects what has been discussed so far.
Wei.
> }
>
> if (fclose(f)) {
> @@ -396,6 +412,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
