From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown) Date: Fri, 10 May 2013 16:07:35 +0100 Message-ID: <518D0D37.2020603@eu.citrix.com> References: <1366985563-10926-1-git-send-email-ian.campbell@citrix.com> <1366986572.3142.116.camel@zakaz.uk.xensource.com> <518CFF7E.1080408@eu.citrix.com> <518D01F4.1010103@eu.citrix.com> <518D04B7.2080706@eu.citrix.com> <518D074B.2040301@eu.citrix.com> <518D0C2B.1020606@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <518D0C2B.1020606@eu.citrix.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: Dave Scott Cc: "xen-devel@lists.xen.org" , Sylvain Munaut , Ian Jackson , Ian Campbell , Roger Pau Monne List-Id: xen-devel@lists.xenproject.org On 10/05/13 16:03, Dave Scott wrote: > On 10/05/13 15:42, George Dunlap wrote: >> On 10/05/13 15:31, Dave Scott wrote: >>> On 10/05/13 15:19, George Dunlap wrote: >>>> On 10/05/13 15:09, Dave Scott wrote: >>>>> On 10/05/13 14:46, George Dunlap wrote: >>>>>> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell wrote: >>>>>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote: >>>>>>>>> Since the intention of that commit was to allow for qdisk backends with no >>>>>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest >>>>>>>>> impact fix appears to be to make that explicit. This should probably be >>>>>>>>> revisited to rationalize the probing. >>>>>>>> What about the remote disk case of blktap ? blktap2.5 supports NBD >>>>>>>> already AFAIK >>>>>>>> and I'm pretty sure I'll hit that same stat issue soon for another >>>>>>>> remote blktap case. >>>>>>> Right, sounds like I should have gone with my first instinct which was: >>>>>>> >>>>>>> 8<------------------------------------------------------------ >>>>>>> >>>>>>> From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001 >>>>>>> From: Ian Campbell >>>>>>> Date: Fri, 26 Apr 2013 12:41:43 +0100 >>>>>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown) >>>>>>> >>>>>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we >>>>>>> expect a device to exist." changed things to only stat the file when the phy >>>>>>> backend was explicitly requested. This broke the case where we are probing and >>>>>>> would normally be able to decide on the phy option. >>>>>> So at the moment qdisk backends aren't checked at all with this -- >>>>>> which means that if you give a path to a file that doesn't exist via, >>>>>> for example, xl cd-insert, things fail in weird ways: >>>>>> >>>>>> 1. In qemu-traditional, the command silently completes; the effect is >>>>>> that the disk currently in the drive is ejected >>>>>> >>>>>> 2. in qemu-upstream, qmp returns an error which is reported. The disk >>>>>> is ejected from the guest, but the xenstore entries are not updated >>>>>> (still contain the old values) >>>>>> >>>>>> It seems like we should probably also at least check if disk_format is RAW. >>>>>> >>>>>> OTOH, I don't seen an option for disk_format to be ceph; is it just >>>>>> listed as "raw" as well? That doesn't seem right... >>>>> AFAICT a ceph disk is in the "raw" format but it uses a custom network >>>>> protocol to actually read and write the blocks. I imagine on the ceph >>>>> servers the disk is stored in a cleverer format, but all qemu/tapdisk >>>>> see are plain blocks with no fancy encoding, no .vhd or .qcow. >>>> Oh, hang on -- does qdisk / tapdisk then just open a plain file? Then it >>>> *is* just a raw file for these purposes; and we should still be able to >>>> call stat() on it. >>> There is no file :) All we have is a "URL"-like thing which looks like: >>> >>> rbd:rbd/foo.img >>> >>> which qemu/tapdisk accesses via a C library called librados. The C >>> library takes the "URL" and makes a network connection to a storage >>> server and reads and writes blocks over TCP/IP. No filesystems are >>> involved, "rbd:rbd/foo.img" doesn't exist in the filesystem. >> Right -- so I think this should be a separate type to RAW. I agree that >> making a new format for each new protocol isn't very scalable -- but >> there should be a "URL" or "VIRTUAL" format to indicate that there is no >> real file. > Something like that would be fine. I agree "format=raw" is a bit odd, > when there's no real "format" involved, only a network protocol. > >> Right now RAW doesn't seem to be talking about the format, but about the >> format string; i.e., "I pass this straight into qemu and it figures out >> what it means." That's not right. >> >> For 4.3 -- I guess the simplest thing to fix the actual bug (xl >> cd-insert giving weird results) is to put a stat in xl_cmdimpl.c before >> calling libxl_cdrom_insert(). > That works for me. Do non-CDROM cases fail in a more reasonable way? Well presumably at some point qemu will return an error if the spec (either a plain file or a network thing) doesn't work properly. Whether that will cause a proper failure for "xl block-attach" or libxl_device_disk_attach() I'm not sure. -George