[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