[libvirt] [PATCH 3/3] storage: Adjustments to disk backend to use stateDir

Fri Jan 16 00:32:56 UTC 2015

On 01/15/2015 10:43 AM, Daniel P. Berrange wrote:

>>
>> I looked through some history and didn't see anywhere that supplied name
>> must match partition name was enforced, but there's a lot of code motion
>> and new features that may cloud the history. Perhaps it's the assumption
>> true in so many other pools that 'name' is what was used to create the
>> file, directory, lv, etc.  There is code that fills in the name if not
>> provided with the partition name (which occurs after Refresh, reload,
>> restart).
> 
> Looking back some more, it seems I did not ever enforce it - I just
> left it as something users had to set the "right way" by convention.
> ie I just expected callers to always use the right "sdaNNN" value
> 

So then should this morph into a fix which causes a failure if a
matching name wasn't passed? Or should we "be nice", "fix it" to
whatever mkpart generates, and not fail?

The check could be done in virStorageBackendDiskMakeDataVol which
refresh, reload, restart already uses in order to assign partition name
(eg, tmp) to vol->name.

The downside of the fail case in that code is we'll need to delete the
partition that just created - something that isn't already done in any
of the failure cases. Trying to compare the name prior to creation
against the assumed next partition # means we'll need to create an
algorithm that finds all the partition numbers and figures out what the
next one will be. They're not necessarily "in order".

As for the target.format (<target...> <format type='linux'> </target>)
I'm inclined to just let it be. Although it does have one use - since
"extended" is a valid virStorageVolFormatDisk value, one could create an
extended partition before all the primary ones are created.

John
>> I'm not quite sure how one could enforce a name given that parted
>> creates the partition name. One would have to know the device name of
>> the pool and the numerical sequence that parted would use (easy perhaps
>> know that perhaps partitions 1 & 2 exist that 3 will be next). However,
>> there's not necessarily a guarantee of that is there?
> 
> For MS-DOS partition tables the partition entry number directly
> corresponds to the device name suffix. eg partition 3 corresponds
> to /dev/sda3 always and is happy to be sparse. eg if you create
> 3 partitions and then delete partition 2, you'll end up with
> /dev/sda1 and /dev/sda3
> 
> Testing with parted the same seems to be true of GPT tables too.
> 
>> Reading the libvirt docs - the implication is that the volume "<name>"
>> must only be unique across the pool, so the following is "valid":
>>
>>     virsh vol-create-as disk-pool vol-linux --format linux 1G
>>
>> So without assuming/describing the naming scheme of the underlying
>> parted how do we enforce that the name (vol-linux) is the same as what
>> parted generates? Or how do we enforce that the name provided ends up
>> being the same as the partition created?  It's kind of a chicken/egg issue.
> 
> Well with disk partitions we know the device name
> 
> eg the pool knows we're operating on '/dev/sda', so when we create
> it can we validate that the volume name is '/dev/sdaNNN' by kjust
> doing a string prefix match
> 
>> Not much is done with the target.format in the backend even though it's
>> described as providing the "partition type". When not provided on the
>> command line like above it defaults to 'none'. Other than for dos label
>> pools that would need to have extended partition before creating a
>> logical partition, there's not much "use" for the field other than to
>> perhaps store "something" for "someone" (until refresh, reload, restart
>> loses it).
> 
> When you create a partition you can specify filesystem type for that
> partition, even though it is somewhat redundant as it is not directly
> connected to the filesystem that you actually mkfs. eg this list of
> types from fdisk
> 
> Command (m for help): l
> 
>  0  Empty           24  NEC DOS         81  Minix / old Lin bf  Solaris        
>  1  FAT12           27  Hidden NTFS Win 82  Linux swap / So c1  DRDOS/sec (FAT-
>  2  XENIX root      39  Plan 9          83  Linux           c4  DRDOS/sec (FAT-
>  3  XENIX usr       3c  PartitionMagic  84  OS/2 hidden C:  c6  DRDOS/sec (FAT-
>  4  FAT16 <32M      40  Venix 80286     85  Linux extended  c7  Syrinx         
>  5  Extended        41  PPC PReP Boot   86  NTFS volume set da  Non-FS data    
>  6  FAT16           42  SFS             87  NTFS volume set db  CP/M / CTOS / .
>  7  HPFS/NTFS/exFAT 4d  QNX4.x          88  Linux plaintext de  Dell Utility   
>  8  AIX             4e  QNX4.x 2nd part 8e  Linux LVM       df  BootIt         
>  9  AIX bootable    4f  QNX4.x 3rd part 93  Amoeba          e1  DOS access     
>  a  OS/2 Boot Manag 50  OnTrack DM      94  Amoeba BBT      e3  DOS R/O        
>  b  W95 FAT32       51  OnTrack DM6 Aux 9f  BSD/OS          e4  SpeedStor      
>  c  W95 FAT32 (LBA) 52  CP/M            a0  IBM Thinkpad hi eb  BeOS fs        
>  e  W95 FAT16 (LBA) 53  OnTrack DM6 Aux a5  FreeBSD         ee  GPT            
>  f  W95 Ext'd (LBA) 54  OnTrackDM6      a6  OpenBSD         ef  EFI (FAT-12/16/
> 10  OPUS            55  EZ-Drive        a7  NeXTSTEP        f0  Linux/PA-RISC b
> 11  Hidden FAT12    56  Golden Bow      a8  Darwin UFS      f1  SpeedStor      
> 12  Compaq diagnost 5c  Priam Edisk     a9  NetBSD          f4  SpeedStor      
> 14  Hidden FAT16 <3 61  SpeedStor       ab  Darwin boot     f2  DOS secondary  
> 16  Hidden FAT16    63  GNU HURD or Sys af  HFS / HFS+      fb  VMware VMFS    
> 17  Hidden HPFS/NTF 64  Novell Netware  b7  BSDI fs         fc  VMware VMKCORE 
> 18  AST SmartSleep  65  Novell Netware  b8  BSDI swap       fd  Linux raid auto
> 1b  Hidden W95 FAT3 70  DiskSecure Mult bb  Boot Wizard hid fe  LANstep        
> 1c  Hidden W95 FAT3 75  PC/IX           be  Solaris boot    ff  BBT            
> 1e  Hidden W95 FAT1 80  Old Minix      
> 
> 
> 
> I did make a start on defining this for the XML:
> 
> /*
>  * XXX: these are basically partition types.
>  *
>  * fdisk has a bazillion partition ID types parted has
>  * practically none, and splits the * info across 3
>  * different attributes.
>  *
>  * So this is a semi-generic set
>  */
> typedef enum {
>     VIR_STORAGE_VOL_DISK_NONE = 0,
>     VIR_STORAGE_VOL_DISK_LINUX,
>     VIR_STORAGE_VOL_DISK_FAT16,
>     VIR_STORAGE_VOL_DISK_FAT32,
>     VIR_STORAGE_VOL_DISK_LINUX_SWAP,
>     VIR_STORAGE_VOL_DISK_LINUX_LVM,
>     VIR_STORAGE_VOL_DISK_LINUX_RAID,
>     VIR_STORAGE_VOL_DISK_EXTENDED,
>     VIR_STORAGE_VOL_DISK_LAST,
> } virStorageVolFormatDisk;
> VIR_ENUM_DECL(virStorageVolFormatDisk)
> 
> 
> but then storage_backend_disk never made use of this information
> 
> I'm not sure how important this is, but if we do want to make use
> of 'format' for the disk pool, this partition types are what I'd
> map it too.
> 
> Regards,
> Daniel
> 