[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [lvm-devel] [PATCH 09/17] Add pv_initialise fn to format_handler interface.



Peter Rajnoha <prajnoha redhat com> writes:

> This is a split-off from the original pv_setup code, a simple refactor.
> It makes the code easier to read and cleans up the interface a bit. So
> now we will call pv_initialise when we initialise a PV only, creating a
> brand new PV. It will also initialise a new format_instance to use later on.

OK, although the parameter lists are huge -- calls to these functions
are plain unreadable. Please consider doing something about it... (this
is a widespread problem in LVM, I know, but that's no excuse to keep not
fixing it).

Yours,
   Petr

> Signed-off-by: Peter Rajnoha <prajnoha redhat com>
Reviewed-by: Petr Rockai <prockai redhat com>

> ---
>  lib/format1/format1.c         |   35 ++++++++++++++------
>  lib/format_pool/format_pool.c |   14 ++++++++
>  lib/format_text/format-text.c |   71 +++++++++++++++++++++++++++++++++++++++++
>  lib/metadata/metadata.h       |   13 +++++++
>  4 files changed, 123 insertions(+), 10 deletions(-)
>
> diff --git a/lib/format1/format1.c b/lib/format1/format1.c
> index 4f66cdc..cb17ef5 100644
> --- a/lib/format1/format1.c
> +++ b/lib/format1/format1.c
> @@ -361,16 +361,15 @@ static int _format1_pv_read(const struct format_type *fmt, const char *pv_name,
>  	return r;
>  }
>  
> -static int _format1_pv_setup(const struct format_type *fmt,
> -			     uint64_t pe_start, uint32_t extent_count,
> -			     uint32_t extent_size,
> -			     unsigned long data_alignment __attribute__((unused)),
> -			     unsigned long data_alignment_offset __attribute__((unused)),
> -			     int pvmetadatacopies __attribute__((unused)),
> -			     uint64_t pvmetadatasize __attribute__((unused)),
> -			     unsigned metadataignore __attribute__((unused)),
> -			     struct dm_list *mdas __attribute__((unused)),
> -			     struct physical_volume *pv, struct volume_group *vg __attribute__((unused)))
> +static int _format1_pv_initialise(const struct format_type * fmt,
> +				  int64_t label_sector __attribute__((unused)),
> +				  uint64_t pe_start,
> +				  int pe_start_locked __attribute__((unused)),
> +				  uint32_t extent_count,
> +				  uint32_t extent_size,
> +				  unsigned long data_alignment __attribute__((unused)),
> +				  unsigned long data_alignment_offset __attribute__((unused)),
> +				  struct physical_volume * pv)
>  {
>  	if (pv->size > MAX_PV_SIZE)
>  		pv->size--;
> @@ -400,6 +399,21 @@ static int _format1_pv_setup(const struct format_type *fmt,
>  	return 1;
>  }
>  
> +static int _format1_pv_setup(const struct format_type *fmt,
> +			     uint64_t pe_start, uint32_t extent_count,
> +			     uint32_t extent_size,
> +			     unsigned long data_alignment __attribute__((unused)),
> +			     unsigned long data_alignment_offset __attribute__((unused)),
> +			     int pvmetadatacopies __attribute__((unused)),
> +			     uint64_t pvmetadatasize __attribute__((unused)),
> +			     unsigned metadataignore __attribute__((unused)),
> +			     struct dm_list *mdas __attribute__((unused)),
> +			     struct physical_volume *pv,
> +			     struct volume_group *vg __attribute__((unused)))
> +{
> +	return _format1_pv_initialise(fmt, -1, 0, 0, 0, vg->extent_size, 0, 0, pv);
> +}
> +
>  static int _format1_lv_setup(struct format_instance *fid, struct logical_volume *lv)
>  {
>  	uint64_t max_size = UINT_MAX;
> @@ -563,6 +577,7 @@ static void _format1_destroy(struct format_type *fmt)
>  
>  static struct format_handler _format1_ops = {
>  	.pv_read = _format1_pv_read,
> +	.pv_initialise = _format1_pv_initialise,
>  	.pv_setup = _format1_pv_setup,
>  	.pv_write = _format1_pv_write,
>  	.lv_setup = _format1_lv_setup,
> diff --git a/lib/format_pool/format_pool.c b/lib/format_pool/format_pool.c
> index 24ccfc2..83f474a 100644
> --- a/lib/format_pool/format_pool.c
> +++ b/lib/format_pool/format_pool.c
> @@ -188,6 +188,19 @@ out:
>  	return NULL;
>  }
>  
> +static int _pool_pv_initialise(const struct format_type *fmt __attribute__((unused)),
> +			       int64_t label_sector __attribute__((unused)),
> +			       uint64_t pe_start __attribute__((unused)),
> +			       int pe_start_locked __attribute__((unused)),
> +			       uint32_t extent_count __attribute__((unused)),
> +			       uint32_t extent_size __attribute__((unused)),
> +			       unsigned long data_alignment __attribute__((unused)),
> +			       unsigned long data_alignment_offset __attribute__((unused)),
> +			       struct physical_volume *pv __attribute__((unused)))
> +{
> +	return 1;
> +}
> +
>  static int _pool_pv_setup(const struct format_type *fmt __attribute__((unused)),
>  			  uint64_t pe_start __attribute__((unused)),
>  			  uint32_t extent_count __attribute__((unused)),
> @@ -295,6 +308,7 @@ static void _pool_destroy(struct format_type *fmt)
>  /* *INDENT-OFF* */
>  static struct format_handler _format_pool_ops = {
>  	.pv_read = _pool_pv_read,
> +	.pv_initialise = _pool_pv_initialise,
>  	.pv_setup = _pool_pv_setup,
>  	.create_instance = _pool_create_instance,
>  	.destroy_instance = _pool_destroy_instance,
> diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
> index 99f691f..c6adedc 100644
> --- a/lib/format_text/format-text.c
> +++ b/lib/format_text/format-text.c
> @@ -1688,6 +1688,76 @@ static int _text_pv_read(const struct format_type *fmt, const char *pv_name,
>  	return 1;
>  }
>  
> +static int _text_pv_initialise(const struct format_type *fmt,
> +			       const int64_t label_sector,
> +			       uint64_t pe_start,
> +			       int pe_start_locked,
> +			       uint32_t extent_count,
> +			       uint32_t extent_size,
> +			       unsigned long data_alignment,
> +			       unsigned long data_alignment_offset,
> +			       struct physical_volume *pv)
> +{
> +	struct text_fid_pv_context *fid_pv_tc;
> +
> +	/*
> +	 * Try to keep the value of PE start set to a firm value if requested.
> +	 * This is usefull when restoring existing PE start value (backups etc.).
> +	 */
> +	if (pe_start_locked)
> +		pv->pe_start = pe_start;
> +
> +	if (!data_alignment)
> +		data_alignment = find_config_tree_int(pv->fmt->cmd,
> +					      "devices/data_alignment",
> +					      0) * 2;
> +
> +	if (set_pe_align(pv, data_alignment) != data_alignment &&
> +	    data_alignment) {
> +		log_error("%s: invalid data alignment of "
> +			  "%lu sectors (requested %lu sectors)",
> +			  pv_dev_name(pv), pv->pe_align, data_alignment);
> +		return 0;
> +	}
> +
> +	if (set_pe_align_offset(pv, data_alignment_offset) != data_alignment_offset &&
> +	    data_alignment_offset) {
> +		log_error("%s: invalid data alignment offset of "
> +			  "%lu sectors (requested %lu sectors)",
> +			  pv_dev_name(pv), pv->pe_align_offset, data_alignment_offset);
> +		return 0;
> +	}
> +
> +	if (pv->pe_align < pv->pe_align_offset) {
> +		log_error("%s: pe_align (%lu sectors) must not be less "
> +			  "than pe_align_offset (%lu sectors)",
> +			  pv_dev_name(pv), pv->pe_align, pv->pe_align_offset);
> +		return 0;
> +	}
> +
> +	if (!pe_start_locked && pv->pe_start < pv->pe_align)
> +		pv->pe_start = pv->pe_align;
> +
> +	if (extent_size)
> +		pv->pe_size = extent_size;
> +
> +	if (extent_count)
> +		pv->pe_count = extent_count;
> +
> +	if ((pv->pe_start + pv->pe_count * pv->pe_size - 1) > (pv->size << SECTOR_SHIFT)) {
> +		log_error("Physical extents end beyond end of device %s.",
> +			   pv_dev_name(pv));
> +		return 0;
> +	}
> +
> +	if (label_sector != -1) {
> +		fid_pv_tc = (struct text_fid_pv_context *) pv->fid->private;
> +		fid_pv_tc->label_sector = label_sector;
> +	}
> +
> +	return 1;
> +}
> +
>  static void _text_destroy_instance(struct format_instance *fid __attribute__((unused)))
>  {
>  }
> @@ -2377,6 +2447,7 @@ void *create_text_context(struct cmd_context *cmd, const char *path,
>  static struct format_handler _text_handler = {
>  	.scan = _text_scan,
>  	.pv_read = _text_pv_read,
> +	.pv_initialise = _text_pv_initialise,
>  	.pv_setup = _text_pv_setup,
>  	.pv_add_metadata_area = _text_pv_add_metadata_area,
>  	.pv_remove_metadata_area = _text_pv_remove_metadata_area,
> diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
> index ed72551..ae8dd9a 100644
> --- a/lib/metadata/metadata.h
> +++ b/lib/metadata/metadata.h
> @@ -244,6 +244,19 @@ struct format_handler {
>  			struct physical_volume * pv, int scan_label_only);
>  
>  	/*
> +	 * Initialise a new PV.
> +	 */
> +	int (*pv_initialise) (const struct format_type * fmt,
> +			      int64_t label_sector,
> +			      uint64_t pe_start,
> +			      int pe_start_locked,
> +			      uint32_t extent_count,
> +			      uint32_t extent_size,
> +			      unsigned long data_alignment,
> +			      unsigned long data_alignment_offset,
> +			      struct physical_volume * pv);
> +
> +	/*
>  	 * Tweak an already filled out a pv ready for importing into a
>  	 * vg.  eg. pe_count is format specific.
>  	 */


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]