Re: [libvirt] [PATCH 2/2] Add fs pool formatting

On 05/13/2010 08:18 PM, David Allan wrote: > * This patch adds the ability to make the filesystem for a filesystem > pool during a pool build. > > The patch adds two new flags, probe and overwrite, to control when > mkfs gets executed. By default, the patch preserves the current > behavior, i.e., if no flags are specified, pool build on a > filesystem pool only makes the directory on which the filesystem > will be mounted. > > typedef enum { > VIR_STORAGE_POOL_BUILD_NEW = 0, /* Regular build from scratch */ > - VIR_STORAGE_POOL_BUILD_REPAIR = 1, /* Repair / reinitialize */ > - VIR_STORAGE_POOL_BUILD_RESIZE = 2 /* Extend existing pool */ > + VIR_STORAGE_POOL_BUILD_REPAIR = (1 << 0), /* Repair / reinitialize */ > + VIR_STORAGE_POOL_BUILD_RESIZE = (1 << 1), /* Extend existing pool */ > + VIR_STORAGE_POOL_BUILD_PROBE = (1 << 2), /* Probe for existing pool */ > + VIR_STORAGE_POOL_BUILD_OVERWRITE = (1 << 3) /* Overwrite data */ Can one specify both probe and overwrite, or are they mutually exclusive? Maybe it makes sense to allow both - don't overwrite the filesystem if it is not already of the correct type, but if it is the correct type, then erase it and start from scratch (contrasted with probe in isolation doing nothing if the right type is present but overwriting on all other types). > +++ b/src/Makefile.am > @@ -747,6 +747,11 @@ libvirt_driver_storage_la_LDFLAGS += -module -avoid-version > endif > libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_SOURCES) > libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_FS_SOURCES) > +if HAVE_LIBBLKID > +libvirt_driver_storage_la_SOURCES += storage/storage_backend_fs_libblkid.c > +libvirt_driver_storage_la_CFLAGS += $(BLKID_CFLAGS) > +libvirt_driver_storage_la_LDFLAGS += $(BLKID_LIBS) Should be LIBADD, not LDFLAGS. > + > +static int > +virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool, > + unsigned int flags) > +{ > + const char *device = NULL, *format = NULL; > + bool ok_to_mkfs = false; > + int ret = -1; Missing virCheckFlags(). > + > + if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) { > + ok_to_mkfs = true; > + } else if (flags & VIR_STORAGE_POOL_BUILD_PROBE && > + virStorageBackendFileSystemProbe(device, format) == > + FILESYSTEM_PROBE_NOT_FOUND) { > + ok_to_mkfs = true; > + } Here's where your logic would need to change - either the two flags are mutually exclusive, and you need to error out if both are given, or the two are both supported, but you need to take into account if both are specified. > --- /dev/null > +++ b/src/storage/storage_backend_fs_libblkid.c > @@ -0,0 +1,97 @@ > +/* > + * storage_backend_fs_libblkid.c: libblkid specific code for > + * filesystem backend > + * > + * Copyright (C) 2007-2010 Red Hat, Inc. Is this just 2010, or did you really borrow significant content from other files with earlier copyrights? If so, list what file you borrowed from. > + > + /* Unfortunately this value comes from the pool definition > + * where it is correctly const char, but liblbkid expects it s/liblbkid/libblkid/ > + * to be char, thus the cast. */ > + names[0] = (char *)format; > + names[1] = NULL; Yeah, unfortunate but necessary. Seems safe, though; libblkid has no business modifying the string. > diff --git a/src/util/virterror.c b/src/util/virterror.c > index 96dd1e7..bd845fb 100644 > --- a/src/util/virterror.c > +++ b/src/util/virterror.c > @@ -1019,6 +1019,18 @@ virErrorMsg(virErrorNumber error, const char *info) > else > errmsg = _("Storage volume not found: %s"); > break; > + case VIR_ERR_STORAGE_PROBE_FAILED: > + if (info == NULL) > + errmsg = _("Storage pool probe failed"); > + else > + errmsg = _("Storage pool probe failed: %s"); > + break; Goody - a merge conflict with my error cleanups. It's a race to see who gets first ACK :) > +++ b/tools/virsh.c > @@ -4668,6 +4668,9 @@ static const vshCmdInfo info_pool_build[] = { > > static const vshCmdOptDef opts_pool_build[] = { > {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name or uuid")}, > + {"probe", VSH_OT_BOOL, 0, N_("do not overwrite an existing " > + "pool of this type")}, > + {"overwrite", VSH_OT_BOOL, 0, N_("overwrite any existing data")}, If we do allow both flags at once, then the wording of "probe" needs to be tweaked. So maybe it's better to make them mutually exclusive. > @@ -4675,7 +4678,7 @@ static int > cmdPoolBuild(vshControl *ctl, const vshCmd *cmd) > { > virStoragePoolPtr pool; > - int ret = TRUE; > + int ret = TRUE, flags = 0; Should flags be unsigned int? Looks like we'll need a v2 patch before I feel good giving an ack.