From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chun Yan Liu" 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 Message-ID: <557AF41B02000066000D4C58@relay2.provo.novell.com> References: <1433906441-3280-1-git-send-email-cyliu@suse.com> <1433906441-3280-3-git-send-email-cyliu@suse.com> <21881.46148.466883.923039@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21881.46148.466883.923039@mariner.uk.xensource.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: george.dunlap@eu.citrix.com, wei.liu2@citrix.com, ian.campbell@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> On 6/12/2015 at 12:16 AM, in message <21881.46148.466883.923039@mariner.uk.xensource.com>, Ian Jackson 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 > >