From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 69C39C6FA82 for ; Fri, 23 Sep 2022 16:43:42 +0000 (UTC) Received: from localhost ([::1]:33132 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oblm8-0007nG-0a for qemu-devel@archiver.kernel.org; Fri, 23 Sep 2022 12:43:40 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:51126) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oblYf-0005n6-1P for qemu-devel@nongnu.org; Fri, 23 Sep 2022 12:29:45 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:48185) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oblYa-0000nd-IG for qemu-devel@nongnu.org; Fri, 23 Sep 2022 12:29:42 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1663950577; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=w+ECCMYmeFrCUa8e9O0tlrIDYq73ueRYyJYgxpDzdBE=; b=jDFj44af/M9M48Tex+U6efSI0TqGwIy0bNGbeuNclZiucUQhn/xgWiZopeeD+TQkdJSUgo zMOwX6jF0Kzxrj2hCW8tsyyeTVbp1hMjJI2xZYnWiEhyWxIlMrPo7ebChGXchZSwF8vge6 ZnhkohyKUlN+qoOXDglGpnE8l1phQs4= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-398-p8cidLZYMQu54NS8zgiXQA-1; Fri, 23 Sep 2022 12:29:26 -0400 X-MC-Unique: p8cidLZYMQu54NS8zgiXQA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 86D90101A528; Fri, 23 Sep 2022 16:29:26 +0000 (UTC) Received: from redhat.com (unknown [10.39.193.141]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8C19140C2064; Fri, 23 Sep 2022 16:29:24 +0000 (UTC) Date: Fri, 23 Sep 2022 18:29:23 +0200 From: Kevin Wolf To: Claudio Fontana Cc: Richard Henderson , Paolo Bonzini , Markus Armbruster , qemu-devel@nongnu.org, dinechin@redhat.com, Gerd Hoffmann , =?iso-8859-1?Q?Marc-Andr=E9?= Lureau , Daniel =?iso-8859-1?Q?P=2E_Berrang=E9?= , Philippe =?iso-8859-1?Q?Mathieu-Daud=E9?= Subject: Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one Message-ID: References: <20220908145308.30282-3-cfontana@suse.de> <062faaa8-064c-f68a-e316-aaacb80efa5a@linaro.org> <3c6cb3ee-2470-654f-c2c4-3449861f9781@suse.de> <8682ad9f-aea8-0419-5ff6-c14493e4e980@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 Received-SPF: pass client-ip=170.10.133.124; envelope-from=kwolf@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Am 23.09.2022 um 16:46 hat Claudio Fontana geschrieben: > On 9/23/22 16:42, Kevin Wolf wrote: > > Am 23.09.2022 um 16:10 hat Claudio Fontana geschrieben: > >> On 9/21/22 13:56, Kevin Wolf wrote: > >>> Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: > >>>> On 9/20/22 18:50, Kevin Wolf wrote: > >>>>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > >>>>>> On 9/8/22 19:10, Claudio Fontana wrote: > >>>>>>> On 9/8/22 18:03, Richard Henderson wrote: > >>>>>>>> On 9/8/22 15:53, Claudio Fontana wrote: > >>>>>>>>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, > >>>>>>>>> return -EINVAL; > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> - block_module_load_one("dmg-bz2"); > >>>>>>>>> - block_module_load_one("dmg-lzfse"); > >>>>>>>>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { > >>>>>>>>> + error_report_err(local_err); > >>>>>>>>> + } > >>>>>>>>> + local_err = NULL; > >>>>>>>>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { > >>>>>>>>> + error_report_err(local_err); > >>>>>>>>> + } > >>>>>>>>> > >>>>>>>>> s->n_chunks = 0; > >>>>>>>>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; > >>>>>>>> > >>>>>>>> I wonder if these shouldn't fail hard if the modules don't exist? > >>>>>>>> Or at least pass back the error. > >>>>>>>> > >>>>>>>> Kevin? > >>>>>> > >>>>>> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg > >>>>>> image is not compressed, "dmg" can function even if the extra dmg-bz > >>>>>> module is not loaded right? > >>>>> > >>>>> Indeed. The code seems to consider that the modules may not be present. > >>>>> The behaviour in these cases is questionable (it seems to silently leave > >>>>> the buffers as they are and return success) > >>> > >>> I think I misunderstood the code here actually. dmg_read_mish_block() > >>> skips chunks of unknown type, so later trying to find them fails and > >>> dmg_co_preadv() returns -EIO. Which is a reasonable return value for > >>> this. > >>> > >>>>> , but the modules are clearly > >>>>> optional. > >>>>> > >>>>>> I'd suspect we should then do: > >>>>>> > >>>>>> if (!block_module_load_one("dmg-bz2", &local_err)) { > >>>>>> if (local_err) { > >>>>>> error_report_err(local_err); > >>>>>> return -EINVAL; > >>>>>> } > >>>>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ > >>>>>> } > >>>>>> > >>>>>> and same for dmg-lzfse...? > >>>>> > >>>>> Actually, I think during initialisation, we should just pass NULL as > >>>>> errp and ignore any errors. > >>>> > >>>> Hmm really? I'd think that if there is an actual error loading the > >>>> module (module is installed, but the loading itself fails due to > >>>> broken module, wrong permissions, I/O errors etc) we would want to > >>>> report that fact as it happens? > >>> > >>> Can we distinguish the two error cases? > >>> > >>> Oooh... Reading the code again carefully, are you returning false > >>> without setting errp if the module just couldn't be found? This is a > >>> surprising interface. > >>> > >>> Yes, I guess then your proposed code is fine (modulo moving > >>> warn_report() somewhere else so that it doesn't complain when the image > >>> doesn't even contain compressed chunks). > >>> > >>>>> When a request would access a block that can't be uncompressed because > >>>>> of the missing module, that's where we can have a warn_report_once() and > >>>>> arguably should fail the I/O request. > >>>>> > >>>>> Kevin > >>>>> > >>>> > >>>> That would mean, moving the > >>>> > >>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") > >>>> > >>>> to the uncompression code and change it to a warn_report_once() right? > >>> > >>> Yeah, though I think this doesn't actually work because we never even > >>> stored the metadata for chunks of unknown type (see above), so we never > >>> reach the uncompression code. > >>> > >>> What misled me initially is this code in dmg_read_chunk(): > >>> > >>> case UDBZ: /* bzip2 compressed */ > >>> if (!dmg_uncompress_bz2) { > >>> break; > >>> } > >>> > >>> I believe this is dead code, it could actually be an assertion. So > >>> if I'm not missing anything, adding the warning there would be useless. > >>> > >>> The other option is moving it into dmg_is_known_block_type() or its > >>> caller dmg_read_mish_block(), then we would detect it during open, which > >>> is probably nicer anyway. > >>> > >>> Kevin > >>> > >>> > >> > >> Hi Kevin, I got a bit lost on whether we have some agreement on where > >> if anywhere to move the check/warning about missing decompression > >> submodules. > >> > >> If that's ok I'd post a V5, and we can rediscuss from the new starting > >> point? > > > > Sure, feel free, though I don't think the code will otherwise change for > > dmg, so we could as well continue here. > > > > My conclusion was that only dmg_read_mish_block() or something called by > > it can know whether compressed blocks exist in the image when the > > modules aren't present. So if we want to make the warning conditional on > > that (and my understanding is correct), this is where a > > warn_report_once() would have to live. > > > > Kevin > > > > I took a look, but I feel a bit too ignorant of the code there, maybe you could move the warning as a patch to the right place after the series? > > Or give me the extra commit needed to move into the right place? Only built and tested with an uncompressed image, so this could use testing with an actual compressed dmg image and the module present or missing, but something like the following should do the trick. Kevin diff --git a/block/dmg.c b/block/dmg.c index 98db18d82a..630cde3416 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -254,6 +254,25 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds, for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) { s->types[i] = buff_read_uint32(buffer, offset); if (!dmg_is_known_block_type(s->types[i])) { + switch (s->types[i]) { + case UDBZ: + warn_report_once("dmg-bzip2 module is missing, accessing bzip2 " + "compressed blocks will result in I/O errors"); + break; + case ULFO: + warn_report_once("dmg-lzfse module is missing, accessing lzfse " + "compressed blocks will result in I/O errors"); + break; + case UDCM: + case UDLE: + /* Comments and last entry can be ignored without problems */ + break; + default: + warn_report_once("Image contains chunks of unknown type %x, " + "accessing them will result in I/O errors", + s->types[i]); + break; + } chunk_count--; i--; offset += 40;