[PATCH] transparent support for pushing qcow2 images to rhev-m

Ian McLeod imcleod at redhat.com
Mon Dec 5 17:45:19 UTC 2011


Jim,

I'd have it read something like this:

** New features

  Add automatic qcow2 format detection to the dc-rhev-image helper
  utility.  This change allows you to use both raw and qcow2 images to
  create templates in a RHEV-M environment.  


On Mon, 2011-12-05 at 17:38 +0100, Jim Meyering wrote:
> Pete Zaitcev wrote:
> 
> > On Fri, 02 Dec 2011 11:36:55 -0600
> > Ian McLeod <imcleod at redhat.com> wrote:
> >
> >> speeds up the NFS file transfer (an internal RHEV-M domain transfer)
> >
> > Did you mean "and internal" in the changelog?
> >
> >> +	if (bswap_32(header.magic) == QCOW_MAGIC &&
> >> +	    bswap_32(header.version) == QCOW_VERSION)
> >> +		return  bswap_64(header.size);
> >> +	else
> >> +		return 0;
> >
> > This is not portable to PPC. Not that we care too much about PPC or ARM,
> > but still, costs us nothing to do it right in this case, so I fixed that up.
> >
> > Please let me know if attached works for you.
> 
> Thanks Ian and Pete.
> However, one piece is missing: a NEWS entry (presumably under
> the subheading of "New features").  Ian can you provide that?
> 
> It'd be nice to include that in this commit.
> 
> > diff --git a/dc-rhev-image.c b/dc-rhev-image.c
> > index 7e9781e..6c32b49 100644
> > --- a/dc-rhev-image.c
> > +++ b/dc-rhev-image.c
> > @@ -34,7 +34,9 @@
> >  #include <stdbool.h>
> >  #include <stdint.h>
> >  #include <unistd.h>
> > +#include <endian.h>
> >  #include <errno.h>
> > +#include <inttypes.h>
> >  #include <time.h>
> >
> >  #include <curl/curl.h>
> > @@ -63,6 +65,9 @@
> >  #define NFSUID 36
> >  #define NFSGID 36
> >
> > +#define QCOW_MAGIC (('Q' << 24) | ('F' << 16) | ('I' << 8) | 0xfb)
> > +#define QCOW_VERSION 2
> > +
> >  #ifndef MIN
> >  # define MIN(a,b) (((a) < (b)) ? (a) : (b))
> >  #endif
> > @@ -137,6 +142,22 @@ struct api_conn {
> >  	char *path;	/* "/rhevm-api" or "/rhevm-api-powershell" */
> >  };
> >
> > +struct qcow_header {
> > +	uint32_t magic;
> > +	uint32_t version;
> > +	uint64_t backing_file_offset;
> > +	uint32_t backing_file_size;
> > +	uint32_t cluster_bits;
> > +	uint64_t size; /* in bytes */
> > +	uint32_t crypt_method;
> > +	uint32_t l1_size;
> > +	uint64_t l1_table_offset;
> > +	uint64_t refcount_table_offset;
> > +	uint32_t refcount_table_clusters;
> > +	uint32_t nb_snapshots;
> > +	uint64_t snapshots_offset;
> > +};
> > +
> >  static void Usage(void)
> >  {
> >  	fprintf(stderr, "ERROR Usage: " TAG " [" TAG ".conf]\n");
> > @@ -156,6 +177,39 @@ static char **env_p;
> >  		uri->token##_len = (len);	\
> >  	} while (0)
> >
> > +/*
> > + * return size of virtual disk inside qcow2 file
> > + * return 0 if file is not qcow2
> > + */
> > +static uint64_t
> > +read_qcow_size(char *image_file)
> 
> That parameter is never modified, so should be "const":
> 
>    read_qcow_size(const char *image_file)
> 
> > +{
> > +	struct qcow_header header;
> > +	FILE *fp;
> > +	size_t n_read;
> > +
> > +	fp = fopen(image_file, "r");
> > +	if (!fp) {
> > +		fprintf(stderr,
> > +		    "ERROR unable to open %s for qcow2 analysis: %s\n",
> > +		    image_file, strerror(errno));
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	n_read = fread(&header, sizeof(header), 1, fp);
> > +	fclose(fp);
> > +
> > +	if (n_read != 1) {
> > +		return 0;
> > +	}
> > +
> > +	if (be32toh(header.magic) == QCOW_MAGIC &&
> > +	    be32toh(header.version) == QCOW_VERSION)
> > +		return be64toh(header.size);
> > +	else
> > +		return 0;
> > +}
> > +
> >  static struct http_uri *huri_parse(struct http_uri *uri, const char *uri_text)
> >  {
> >  	const char *p, *temp;
> > @@ -1377,7 +1431,8 @@ static void gen_uuids(struct id_pack *idp)
> >  }
> >
> >  static void spitovf(const struct config *cfg, const struct stor_dom *sd,
> > -    const char *poolid, const struct id_pack *idp, off_t vol_size)
> > +    const char *poolid, const struct id_pack *idp, off_t vol_size,
> > +    bool qcow2)
> >  {
> >  	time_t now;
> >  	struct tm now_tm;
> > @@ -1523,12 +1578,21 @@ static void spitovf(const struct config *cfg, const struct stor_dom *sd,
> >  	    BAD_CAST "ovf:format",
> >  	    BAD_CAST "http://www.vmware.com/technical-resources/interfaces/vmdk_access.html");
> >  	if (rc < 0) goto err_xml;
> > -	rc = xmlTextWriterWriteAttribute(writer,
> > -	    BAD_CAST "ovf:volume-format", BAD_CAST "RAW");
> > -	if (rc < 0) goto err_xml;
> > -	rc = xmlTextWriterWriteAttribute(writer,
> > -	    BAD_CAST "ovf:volume-type", BAD_CAST "Preallocated");
> > -	if (rc < 0) goto err_xml;
> > +	if (qcow2) {
> > +		rc = xmlTextWriterWriteAttribute(writer,
> > +		    BAD_CAST "ovf:volume-format", BAD_CAST "COW");
> > +		if (rc < 0) goto err_xml;
> > +		rc = xmlTextWriterWriteAttribute(writer,
> > +		    BAD_CAST "ovf:volume-type", BAD_CAST "Sparse");
> > +		if (rc < 0) goto err_xml;
> > +	} else {
> > +		rc = xmlTextWriterWriteAttribute(writer,
> > +		    BAD_CAST "ovf:volume-format", BAD_CAST "RAW");
> > +		if (rc < 0) goto err_xml;
> > +		rc = xmlTextWriterWriteAttribute(writer,
> > +		    BAD_CAST "ovf:volume-type", BAD_CAST "Preallocated");
> > +		if (rc < 0) goto err_xml;
> > +	}
> >  	rc = xmlTextWriterWriteAttribute(writer,
> >  	    BAD_CAST "ovf:disk-interface", BAD_CAST "VirtIO");
> >  	if (rc < 0) goto err_xml;
> > @@ -1848,6 +1912,7 @@ static void copyimage(const struct config *cfg, const struct id_pack *idp,
> >  	char *imgsrc, *imgdst, *imgmeta;
> >  	time_t now;
> >  	off_t vol_size;
> > +	uint64_t qcow_size;
> >  	FILE *fp;
> >
> >  	if (geteuid() == 0) {
> > @@ -1896,12 +1961,17 @@ static void copyimage(const struct config *cfg, const struct id_pack *idp,
> >  	/* No error processing here, it exits on error. */
> >  	copy_file_preserving(imgsrc, imgdst);
> >
> > -	if (stat(imgdst, &statb) < 0) {
> > -		fprintf(stderr, "ERROR failed to stat %s: %s\n",
> > -		    imgdst, strerror(errno));
> > -		exit(EXIT_FAILURE);
> > +	qcow_size = read_qcow_size(imgsrc);
> > +	if (qcow_size) {
> > +		vol_size = qcow_size;
> > +	} else {
> > +		if (stat(imgdst, &statb) < 0) {
> > +			fprintf(stderr, "ERROR failed to stat %s: %s\n",
> > +			    imgdst, strerror(errno));
> > +			exit(EXIT_FAILURE);
> > +		}
> > +		vol_size = statb.st_size;
> 
> (not new with this change, so you needn't change anything)
> One nit: technically, stat.st_size is valid only if the stat'ed
> file is a regular file (S_ISREG).  I.e., if it's a device or char- or
> block-dev, then that number will be meaningless, and anything that
> tries to use it may well fail mysteriously.  Better to reject early.
> 
> >  	}
> > -	vol_size = statb.st_size;
> >
> >  	rc = asprintf(&imgmeta, "%s.meta", imgdst);
> >  	if (rc < 0)
> > @@ -1918,7 +1988,10 @@ static void copyimage(const struct config *cfg, const struct id_pack *idp,
> >  	fprintf(fp, "VOLTYPE=LEAF\n");
> >  	fprintf(fp, "CTIME=%llu\n", (long long)now);
> >  	/* saved template has FORMAT=COW */
> > -	fprintf(fp, "FORMAT=RAW\n");
> > +	if (qcow_size)
> > +		fprintf(fp, "FORMAT=COW\n");
> > +	else
> > +		fprintf(fp, "FORMAT=RAW\n");
> 
> How about this instead?  Remove duplication and save two lines:
> 
>         char const *qcow_type = qcow_size ? "COW" : "RAW";
>         fprintf(fp, "FORMAT=%s\n", qcow_type);
> 
> >  	fprintf(fp, "IMAGE=%s\n", idp->image);
> >  	fprintf(fp, "DISKTYPE=1\n");
> >  	fprintf(fp, "PUUID=%s\n", NULID);
> > @@ -1935,7 +2008,7 @@ static void copyimage(const struct config *cfg, const struct id_pack *idp,
> >  		exit(EXIT_FAILURE);
> >  	}
> >
> > -	spitovf(cfg, sd, poolid, idp, vol_size);
> > +	spitovf(cfg, sd, poolid, idp, vol_size, qcow_size != 0);
> >
> >  	free(imgmeta);
> >  	free(imgdst);

-- 
Ian McLeod - Red Hat
office: 312.660.3539
mobile: 312.899.6736
rh internal: (81) 33539



More information about the iwhd-devel mailing list