From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl Date: Wed, 21 May 2014 17:12:01 +0100 Message-ID: <537CD051.4000901@citrix.com> References: <1400524597-6090-1-git-send-email-andryuk@aero.org> <537A80BB.6010609@citrix.com> <21372.52866.217052.802311@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21372.52866.217052.802311@mariner.uk.xensource.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: Ian Jackson Cc: Jason Andryuk , Ian Campbell , Stefano Stabellini , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 21/05/14 17:04, Ian Jackson wrote: > Andrew Cooper writes ("Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl"): >> On 19/05/2014 19:36, Jason Andryuk wrote: >>> toolstack_save data is written to a temporary file in libxl and read >>> back in libxl-save-helper. The file position must be reset prior to >>> reading the file, which is done in libxl-save-helper with lseek. >>> >>> lseek is unsupported for pipes and sockets, so a wrapper passing such an >>> fd to libxl-save-helper fails the lseek. Moving the lseek to libxl >>> avoids the error, allowing the save to continue. >>> >>> Signed-off-by: Jason Andryuk >>> Reviewed-by: Andrew Cooper >> Ian: As noted previously, this should be a backport candidate. > We normally do backports only for bugs (and not even all bugs). I'm > afraid that I don't see how the current arrangements are a bug. > > See my other mail. Perhaps the answer to that will explain to me how > the current are a bug, rather than simply less convenient and flexible > than they could be. > > Thanks, > Ian. It is a layering violation. libxl-save-helper cannot assume that the fd is a file, so shouldn't seek on it. libxl however should prepare a "socket-like" fd to the save-helper, which means rewinding the file itself. As indicated, "toolstack_data" is heading very much in the direction of /dev/null with the migration v2 patches, as it itself is a gross layering violation. If you don't deem this worthy for backport, its not the end of the world. ~Andrew