From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH V4 2/7] libxl_read_file_contents: add new entry to read sysfs file Date: Fri, 12 Jun 2015 16:11:50 +0100 Message-ID: <21882.63158.476825.722466@mariner.uk.xensource.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> <557AF41B02000066000D4C58@relay2.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <557AF41B02000066000D4C58@relay2.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Chun Yan Liu 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 Chun Yan Liu writes ("Re: [Xen-devel] [PATCH V4 2/7] libxl_read_file_contents: add new entry to read sysfs file"): > <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 > > 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. I'm afraid that's not really a complete explanation. Guessing, I think your calling code expected a trailing nul. If you want to make an API which is useable for such code, and not intended for byte blocks, that's fine, but you must then always nul-terminate (not only if the data was less than 4k) and your new function probably doesn't want to return a length at all. > > 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. > > > +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? I meant to move it to libxl_internal.h. Subject to consideration of what its API ought to be. Ian.