From: "Chun Yan Liu" <cyliu@suse.com>
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: george.dunlap@eu.citrix.com, wei.liu2@citrix.com,
ian.campbell@citrix.com, xen-devel@lists.xen.org
Subject: Re: [PATCH V4 2/7] libxl_read_file_contents: add new entry to read sysfs file
Date: Fri, 12 Jun 2015 01:00:43 -0600 [thread overview]
Message-ID: <557AF41B02000066000D4C58@relay2.provo.novell.com> (raw)
In-Reply-To: <21881.46148.466883.923039@mariner.uk.xensource.com>
>>> On 6/12/2015 at 12:16 AM, in message
<21881.46148.466883.923039@mariner.uk.xensource.com>, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
> Chunyan Liu writes ("[Xen-devel] [PATCH V4 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 patch is roughly right, but:
>
> > -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 is_sysfs_file)
>
> I would prefer a functional rather than contextual name for the
> variabvle is_sysfs_file. How about `tolerate_shrinking_file' ?
OK.
>
> > @@ -360,15 +362,16 @@ int libxl_read_file_contents(libxl_ctx *ctx, const
> char *filename,
> >
> > if (stab.st_size && data_r) {
> > data = malloc(datalen);
> > + memset(data, 0, datalen);
>
> What is this for ?
I found sometimes reading sysfs file contents, at the end, there is
some random character. With this line, there is no problem then.
>
> > if (!data) goto xe;
> >
> > rs = fread(data, 1, datalen, f);
> > - if (rs != datalen) {
> > + if (rs != datalen && !(feof(f) && is_sysfs_file)) {
> > if (ferror(f))
> > LOGE(ERROR, "failed to read %s", filename);
> > else if (feof(f))
>
> I think it would be better to handle the special case here, with
> something like
> if (!tolerate_shrinking_file)
> error stuff
> else {
> assert(datalen_r);
> and to move the `goto xe' into the individual branches.
OK. Will update.
>
>
> Is there any risk that the file is actually bigger than advertised,
> rather than smaller ?
For sysfs file, couldn't be bigger.
>
> > diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
> > index 1c1761d..7639662 100644
> > --- a/tools/libxl/libxl_utils.h
> > +++ b/tools/libxl/libxl_utils.h
> ...
> > +int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
> > + void **data_r, int *datalen_r);
>
> I think this is a function with sufficiently odd semantics, and a
> sufficiently internal purpose, that it should probably not be exposed
> in the API.
So move to libxl_internal.h? Or not hacking here but just adding an internal
function in libxl_pvusb.c with repeated codes?
>
> Ian
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
next prev parent reply other threads:[~2015-06-12 7:00 UTC|newest]
Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-10 3:20 [PATCH V4 0/7] xen pvusb toolstack work Chunyan Liu
2015-06-10 3:20 ` [PATCH V4 1/7] libxl: export some functions for pvusb use Chunyan Liu
2015-06-11 16:08 ` Ian Jackson
2015-06-11 16:28 ` Wei Liu
2015-06-12 15:14 ` Ian Jackson
2015-06-10 3:20 ` [PATCH V4 2/7] libxl_read_file_contents: add new entry to read sysfs file Chunyan Liu
2015-06-11 16:16 ` Ian Jackson
2015-06-12 7:00 ` Chun Yan Liu [this message]
2015-06-12 15:11 ` Ian Jackson
2015-06-10 3:20 ` [PATCH V4 3/7] libxl: add pvusb API Chunyan Liu
2015-06-11 15:00 ` Juergen Gross
2015-06-11 16:07 ` Ian Jackson
2015-06-11 16:42 ` Ian Jackson
2015-06-12 7:39 ` Chun Yan Liu
2015-06-12 8:06 ` Chun Yan Liu
2015-06-12 11:22 ` Ian Jackson
2015-06-15 14:17 ` George Dunlap
2015-06-15 14:25 ` Jürgen Groß
2015-06-15 14:34 ` George Dunlap
2015-06-15 18:26 ` Juergen Gross
2015-06-16 10:30 ` George Dunlap
2015-06-16 10:51 ` Juergen Gross
2015-06-16 11:11 ` George Dunlap
2015-06-16 11:19 ` Juergen Gross
2015-06-16 11:23 ` George Dunlap
2015-06-16 11:44 ` Ian Jackson
2015-06-17 11:24 ` Ian Campbell
2015-06-18 11:50 ` George Dunlap
2015-06-18 12:08 ` George Dunlap
2015-06-18 13:03 ` Juergen Gross
2015-06-22 13:29 ` Proposed plan for libxl USB interface (was Re: [PATCH V4 3/7] libxl: add pvusb API) George Dunlap
2015-06-22 14:14 ` Juergen Gross
2015-06-22 14:22 ` Ian Jackson
2015-06-23 2:42 ` Chun Yan Liu
2015-06-23 2:43 ` Chun Yan Liu
2015-06-23 2:44 ` Chun Yan Liu
2015-06-16 10:41 ` [PATCH V4 3/7] libxl: add pvusb API Ian Jackson
2015-06-16 10:56 ` Jürgen Groß
2015-06-16 11:03 ` George Dunlap
2015-06-16 11:10 ` Ian Jackson
2015-06-16 11:25 ` Juergen Gross
2015-06-16 11:45 ` George Dunlap
2015-06-16 12:02 ` Ian Jackson
2015-06-16 13:19 ` George Dunlap
2015-06-16 13:32 ` Juergen Gross
2015-06-16 13:37 ` [PATCH V4 3/7] libxl: add pvusb API [and 1 more messages] Ian Jackson
2015-06-16 14:41 ` George Dunlap
2015-06-16 15:58 ` Sander Eikelenboom
2015-06-16 15:59 ` Ian Jackson
2015-06-16 16:34 ` George Dunlap
2015-06-17 3:59 ` Juergen Gross
2015-06-17 10:27 ` George Dunlap
2015-06-18 6:24 ` Chun Yan Liu
2015-06-16 11:45 ` [PATCH V4 3/7] libxl: add pvusb API Ian Jackson
2015-06-16 13:06 ` Juergen Gross
2015-06-16 13:09 ` George Dunlap
2015-06-16 13:23 ` Juergen Gross
2015-06-16 13:29 ` George Dunlap
2015-06-16 13:49 ` Juergen Gross
2015-06-16 14:06 ` George Dunlap
2015-06-16 14:20 ` Juergen Gross
2015-06-16 14:37 ` George Dunlap
2015-06-17 11:34 ` Ian Campbell
2015-06-17 11:40 ` Juergen Gross
2015-06-18 6:20 ` Chun Yan Liu
2015-06-18 7:02 ` Juergen Gross
2015-06-18 8:50 ` Ian Campbell
2015-06-18 13:02 ` Juergen Gross
2015-06-16 15:38 ` George Dunlap
2015-06-16 11:01 ` George Dunlap
2015-06-16 11:12 ` Ian Jackson
2015-06-16 11:21 ` George Dunlap
2015-06-16 16:32 ` Ian Jackson
2015-06-16 16:39 ` George Dunlap
2015-06-16 16:51 ` Ross Philipson
2015-06-17 4:03 ` Jürgen Groß
2015-06-17 13:48 ` Ross Philipson
2015-06-15 17:47 ` George Dunlap
2015-06-23 10:18 ` Chun Yan Liu
2015-06-23 11:29 ` George Dunlap
2015-06-24 2:26 ` Chun Yan Liu
2015-06-10 3:20 ` [PATCH V4 4/7] libxl: add libxl_device_usb_assignable_list API Chunyan Liu
2015-06-10 3:20 ` [PATCH V4 5/7] xl: add pvusb commands Chunyan Liu
2015-06-12 7:37 ` Juergen Gross
2015-06-12 8:03 ` Chun Yan Liu
2015-06-12 8:22 ` Juergen Gross
2015-06-10 3:20 ` [PATCH V4 6/7] xl: add usb-assignable-list command Chunyan Liu
2015-06-10 3:20 ` [PATCH V4 7/7] domcreate: support pvusb in configuration file Chunyan Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=557AF41B02000066000D4C58@relay2.provo.novell.com \
--to=cyliu@suse.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=george.dunlap@eu.citrix.com \
--cc=ian.campbell@citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.