From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42412) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gZaWY-0008Tf-5f for qemu-devel@nongnu.org; Wed, 19 Dec 2018 07:00:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gZaWU-0003vy-40 for qemu-devel@nongnu.org; Wed, 19 Dec 2018 07:00:26 -0500 References: <1545149484-4929-1-git-send-email-thuth@redhat.com> <878t0mbs79.fsf@dusky.pond.sub.org> From: Thomas Huth Message-ID: Date: Wed, 19 Dec 2018 13:00:09 +0100 MIME-Version: 1.0 In-Reply-To: <878t0mbs79.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [QEMU PATCH] block: Remove blk_attach_dev_legacy() / legacy_dev code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-block@nongnu.org, Kevin Wolf , Max Reitz , Stefano Stabellini , Anthony Perard , xen-devel@lists.xenproject.org, qemu-devel@nongnu.org On 2018-12-18 19:33, Markus Armbruster wrote: > Thomas Huth writes: > >> The last user of blk_attach_dev_legacy() is the code in xen_disk.c. >> It passes a pointer to a XenBlkDev as second parameter. XenBlkDev >> is derived from XenDevice which in turn is derived from DeviceState >> since commit 3a6c9172ac5951e ("xen: create qdev for each backend device"). >> Thus the code can also simply use blk_attach_dev() with a pointer >> to the DeviceState instead. >> So we can finally remove all code related to the "legacy_dev" flag, too, >> and turn the related "void *" in block-backend.c into "DeviceState *" >> to fix some of the remaining TODOs there. >> >> Signed-off-by: Thomas Huth >> --- >> Note: I haven't tested the Xen code since I don't have a working Xen >> installation at hand. I'd appreciate if someone could check it... [...] >> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c >> index 36eff94..9605caf 100644 >> --- a/hw/block/xen_disk.c >> +++ b/hw/block/xen_disk.c >> @@ -801,7 +801,9 @@ static int blk_connect(struct XenDevice *xendev) >> * so we can blk_unref() unconditionally */ >> blk_ref(blkdev->blk); >> } >> - blk_attach_dev_legacy(blkdev->blk, blkdev); >> + if (blk_attach_dev(blkdev->blk, DEVICE(blkdev)) < 0) { >> + return -1; >> + } > > Other error returns in this function call xen_pv_printf() first. Should > this one, too? Only some of them do a xen_pv_printf() first, there are also many that don't. blk_attach_dev() currently only returns an error if a device has already been attach - which should simply never happen here, so a printf currently does not seem to be justified to me. Alternatively, we could also abort() here instead, I think. I'll leave that decision up to the Xen maintainers... Thomas