From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60461) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XV0Sb-0006q4-As for qemu-devel@nongnu.org; Fri, 19 Sep 2014 11:51:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XV0SW-000892-6x for qemu-devel@nongnu.org; Fri, 19 Sep 2014 11:51:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64746) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XV0SV-00088i-Rr for qemu-devel@nongnu.org; Fri, 19 Sep 2014 11:50:56 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s8JFonYP029274 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 19 Sep 2014 11:50:50 -0400 Message-ID: <541C50D3.9030109@redhat.com> Date: Fri, 19 Sep 2014 11:50:43 -0400 From: John Snow MIME-Version: 1.0 References: <1411063146-24058-1-git-send-email-jsnow@redhat.com> <1411063146-24058-2-git-send-email-jsnow@redhat.com> <87zjdwqajp.fsf@blackfin.pond.sub.org> In-Reply-To: <87zjdwqajp.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v2 1/3] blockdev: Add function to search for orphaned drives List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org On 09/19/2014 04:28 AM, Markus Armbruster wrote: > John Snow writes: > >> Signed-off-by: John Snow >> --- >> blockdev.c | 19 +++++++++++++++++++ >> include/sysemu/blockdev.h | 1 + >> vl.c | 5 +++++ >> 3 files changed, 25 insertions(+) >> >> diff --git a/blockdev.c b/blockdev.c >> index b361fbb..5e7c93a 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -166,6 +166,25 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit) >> return NULL; >> } >> >> +DriveInfo *drive_check_orphaned(void) >> +{ >> + DriveInfo *dinfo; >> + DriveInfo *ret = NULL; >> + >> + QTAILQ_FOREACH(dinfo, &drives, next) { >> + /* If dev is NULL, it has no device attached. >> + * If drv is non-NULL, it has a file attached. >> + * If both conditions are true, it is possibly an oversight. */ > > Suggest to spell out dinfo->bdrv->dev and dinfo->bdrv->drv. > > "File attached" is imprecise. BDS member drv is non-null betwen > bdrv_open() and bdrv_close(). A BDS with null drv means "empty", in the > sense of "no medium". > > >> + if ((dinfo->bdrv->dev == NULL) && (dinfo->bdrv->drv != NULL)) { >> + fprintf(stderr, "Orphaned drive: id=%s,if=%s,file=%s\n", >> + dinfo->id, if_name[dinfo->type], dinfo->bdrv->filename); >> + ret = dinfo; >> + } >> + } > > Please prefix "Warning:" to make the nature of this message more > explicit. > > "Orphaned drive" might not be obvious to all users, but it's concise, > and no worse than the "has no peer" we use for NICs. > > You warn when a non-empty drive is not used by a device model. > > This warns when you create one with -drive if=none for future use in the > monitor. I guess that's fine. > > It doesn't warn for empty drives. I doubt "empty" should make a > difference. I didn't want it to warn about things like the SD drive and the floppy drive, created by default, but you're right. Your suggestion below fixes this problem. > I think the condition to check is "has the board failed to pick up a > drive that is meant to be picked up by the board": > > dinfo->type != IF_NONE && !dinfo->bdrv->dev > > I guess this can warn about default drives, because we blindly add them > whether the boards wants them or not. Stupidest solution that could > possibly work: add a flag to DriveInfo to suppress the warning for them. Hm. Actually, the default drives are added by their specific interfaces. IF_IDE, IF_FLOPPY and IF_SD. This is a good improvement. (Well, cdrom is actually added via block_default_type which is usually unset, and happens to coincide with IF_IDE.) > Better solution: don't add them unless the board wants them. I tried > that before, but my solution[*] went nowhere. If you're interested in > trying again, let me know, and I'll explain. > >> + >> + return ret; >> +} >> + >> DriveInfo *drive_get_by_index(BlockInterfaceType type, int index) >> { >> return drive_get(type, >> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h >> index 23a5d10..25d52d2 100644 >> --- a/include/sysemu/blockdev.h >> +++ b/include/sysemu/blockdev.h >> @@ -46,6 +46,7 @@ struct DriveInfo { >> }; >> >> DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit); >> +DriveInfo *drive_check_orphaned(void); >> DriveInfo *drive_get_by_index(BlockInterfaceType type, int index); >> int drive_get_max_bus(BlockInterfaceType type); >> DriveInfo *drive_get_next(BlockInterfaceType type); >> diff --git a/vl.c b/vl.c >> index 5db0d08..e095bcd 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -4457,6 +4457,11 @@ int main(int argc, char **argv, char **envp) >> if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0) >> exit(1); >> >> + /* anybody left over? */ >> + if (drive_check_orphaned()) { >> + fprintf(stderr, "Warning: found drives without a backing device.\n"); >> + } >> + >> net_check_clients(); >> >> ds = init_displaystate(); > > > [*] https://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg02993.html >