[et-mgmt-tools] [PATCH] bugzillaID:329091

[Daniel P. Berrange]

On Wed, Oct 17, 2007 at 01:49:05PM -0400, Cole Robinson wrote:
> S.Sakamoto wrote:
> > Hi
> > 
> > I make a patch of BugzillaID-329091.
> > https://bugzilla.redhat.com/show_bug.cgi?id=329091
> > 
> > Thanks,
> > Shigeki Sakamoto.
> > 
> > ----------------------------------------------------------------------
> > diff -r 531b73491ac2 virt-install
> > --- a/virt-install	Wed Oct 10 14:24:48 2007 -0400
> > +++ b/virt-install	Tue Oct 16 16:01:29 2007 +0900
> > @@ -15,6 +15,7 @@
> >  
> >  
> >  import os, sys, string
> > +from stat import *
> >  from optparse import OptionParser, OptionValueError
> >  import subprocess
> >  import logging
> > @@ -48,10 +49,10 @@ def get_disk(disk, size, sparse, guest, 
> >          if not size is None:
> >              msg = _("Please enter the path to the file you would like to use for storage. It will have size %sGB.") %(size,)
> >          disk = cli.prompt_for_input(msg, disk)
> > -        if os.path.exists(disk) and os.path.isfile(disk):
> > +        if os.path.exists(disk) and ( S_ISBLK(os.stat(disk)[ST_MODE]) or S_ISREG(os.stat(disk)[ST_MODE]) ):
> >              while 1:
> >                  retryFlg = False
> > -                warnmsg = _("You are going to overwrite file '%s'!\n") % disk
> > +                warnmsg = _("You are going to overwrite '%s'!\n") % disk
> >                  res = cli.prompt_for_input(warnmsg + _("Do you really want to use the file (yes or no)? "))
> >                  try:
> >                      if cli.yes_or_no(res) is True:
> > ----------------------------------------------------------------------
> 
> Hello,
> 
> I don't think this is the right approach. From the bug report, I agree
> that we shouldn't make it so easy to ruin a block device without some
> kind of precaution, but the right way to do it would be to have something
> that would require the user to explicitly specify the type of storage
> desired. Such as --file be just for file based storage, and add a --blkdev
> or some such option to specify device based storage. Or maintain --file as
> the storage location and add a --disktype option.
> 
> Unfortunately this would break the existing interface, but so would
> this patch, which would require interaction anytime someone wanted
> to specify a device for storage.

Yep, this is not acceptable. If we force a prompt for all block devices,
then the user will just end up using the override flag all the time,
defeating the purpose of checking. Using a separate --blkdev flag does
not solve the problem either, since they could still easily overwrite
the host filesystem.

> Anyone else have an opinion on the right direction to go? I think
> having an explicit way to specify storage would be handy anyways.

Nope, this doesn't solve anything. The problem is that given a block
device path /dev/sdN, there is no easy way to determine if it is already
used in the host. You can check if its mounted, but that's useless of
the volume is used as Swap. It also misses if the device is in fact a
physical volume in LVM. It also misses if the device is setup via dmcrypt.

It is frankly a loosing battle to try & guess if a device is in use in
the host. Xen does a minimal attempt by looking to see if it is mounted
but that misses the swap & LVM & device mapper scenarios. I don't see us
being able todo any better in virt-install. 


The only viable long term approach is to develop the libvirt storage APIs,
allowing the admin to explicitly design certain devices as being available
for guest usage ahead of time. That would let virt-install validate paths
given to it for sanity

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|