| Message ID | 1461191272-17802-1-git-send-email-Janne.Karhunen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 04/20 15:27, Janne Karhunen wrote: > From: Janne Karhunen <janne.karhunen@gmail.com> > > Replace hardcoded vmdk description file hardware version flag with > a user definable version. Hi Janne, Since this is a new feature, please explain why this change is desired (i.e. the necessity.) > > Signed-off-by: Janne Karhunen <Janne.Karhunen@gmail.com> > --- > block/vmdk.c | 20 ++++++++++---------- > include/block/block_int.h | 3 +-- > qemu-doc.texi | 4 ++-- > 3 files changed, 13 insertions(+), 14 deletions(-) > > diff --git a/block/vmdk.c b/block/vmdk.c > index 45f9d3c..1436785 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -1829,8 +1829,8 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) > int64_t total_size = 0, filesize; > char *adapter_type = NULL; > char *backing_file = NULL; > + char *hw_version = NULL; > char *fmt = NULL; > - int flags = 0; > int ret = 0; > bool flat, split, compress; > GString *ext_desc_lines; > @@ -1861,7 +1861,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) > "# The Disk Data Base\n" > "#DDB\n" > "\n" > - "ddb.virtualHWVersion = \"%d\"\n" > + "ddb.virtualHWVersion = \"%s\"\n" > "ddb.geometry.cylinders = \"%" PRId64 "\"\n" > "ddb.geometry.heads = \"%" PRIu32 "\"\n" > "ddb.geometry.sectors = \"63\"\n" > @@ -1878,9 +1878,8 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) > BDRV_SECTOR_SIZE); > adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE); > backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE); > - if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) { > - flags |= BLOCK_FLAG_COMPAT6; > - } > + hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION); > + > fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT); > if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false)) { > zeroed_grain = true; > @@ -2001,7 +2000,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) > fmt, > parent_desc_line, > ext_desc_lines->str, > - (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4), > + hw_version, > total_size / > (int64_t)(63 * number_heads * BDRV_SECTOR_SIZE), > number_heads, > @@ -2047,6 +2046,7 @@ exit: > } > g_free(adapter_type); > g_free(backing_file); > + g_free(hw_version); > g_free(fmt); > g_free(desc); > g_free(path); > @@ -2292,10 +2292,10 @@ static QemuOptsList vmdk_create_opts = { > .help = "File name of a base image" > }, > { > - .name = BLOCK_OPT_COMPAT6, > - .type = QEMU_OPT_BOOL, > - .help = "VMDK version 6 image", > - .def_value_str = "off" > + .name = BLOCK_OPT_HWVERSION, > + .type = QEMU_OPT_STRING, > + .help = "VMDK hardware version", > + .def_value_str = "4" > }, For backward compatibility reason, we shouldn't remove a pre-existing option without solid justification and probably a grace period. I suggest adding the hwversion option in addition, and document and check it as exclusive with the compat6 flag. Fam > { > .name = BLOCK_OPT_SUBFMT, > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 10d8759..0852316 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -38,12 +38,11 @@ > #include "qemu/throttle.h" > > #define BLOCK_FLAG_ENCRYPT 1 > -#define BLOCK_FLAG_COMPAT6 4 > #define BLOCK_FLAG_LAZY_REFCOUNTS 8 > > #define BLOCK_OPT_SIZE "size" > #define BLOCK_OPT_ENCRYPT "encryption" > -#define BLOCK_OPT_COMPAT6 "compat6" > +#define BLOCK_OPT_HWVERSION "hwversion" > #define BLOCK_OPT_BACKING_FILE "backing_file" > #define BLOCK_OPT_BACKING_FMT "backing_fmt" > #define BLOCK_OPT_CLUSTER_SIZE "cluster_size" > diff --git a/qemu-doc.texi b/qemu-doc.texi > index 79141d3..79f4168 100644 > --- a/qemu-doc.texi > +++ b/qemu-doc.texi > @@ -691,8 +691,8 @@ Supported options: > @table @code > @item backing_file > File name of a base image (see @option{create} subcommand). > -@item compat6 > -Create a VMDK version 6 image (instead of version 4) > +@item hwversion > +Specify vmdk hardware version (default 4) > @item subformat > Specifies which VMDK subformat to use. Valid options are > @code{monolithicSparse} (default), > -- > 1.9.1 > >
On Wed, Apr 20, 2016 at 7:22 PM, Fam Zheng <famz@redhat.com> wrote: >> Replace hardcoded vmdk description file hardware version flag with >> a user definable version. > > Hi Janne, > > Since this is a new feature, please explain why this change is desired (i.e. > the necessity.) See here what it controls for vmware: https://kb.vmware.com/selfservice/microsites/search.do?languag e=en_US&cmd=displayKC&externalId=1003746 Seems that the vmware cluster I'm using is configured to eat vmdks with only certain virtual hardware versions specified as being supported, so this patch allows me to define that version. Besides that, I've read that to mean that it's a hint from me (as the vmdk author) to the cluster that this is the vmware virtual hardware version I have tested this vmdk to run with. It's actually relatively useful metadata as such. In other words, without the patch not a single qemu-img generated vmdk is 'good' for the system I'm using. -- Janne
On Wed, Apr 20, 2016 at 7:22 PM, Fam Zheng <famz@redhat.com> wrote: > For backward compatibility reason, we shouldn't remove a pre-existing option > without solid justification and probably a grace period. I suggest adding the > hwversion option in addition, and document and check it as exclusive with the > compat6 flag. That's certainly doable and kind of makes sense, but I'm not entirely sure that compat6 flag makes any sense to begin with. Does it? -- Janne
On Wed, 04/20 20:02, Janne Karhunen wrote: > On Wed, Apr 20, 2016 at 7:22 PM, Fam Zheng <famz@redhat.com> wrote: > > >> Replace hardcoded vmdk description file hardware version flag with > >> a user definable version. > > > > Hi Janne, > > > > Since this is a new feature, please explain why this change is desired (i.e. > > the necessity.) > > See here what it controls for vmware: > > https://kb.vmware.com/selfservice/microsites/search.do?languag > e=en_US&cmd=displayKC&externalId=1003746 > > Seems that the vmware cluster I'm using is configured to eat vmdks > with only certain virtual hardware versions specified as being > supported, so this patch allows me to define that version. Besides > that, I've read that to mean that it's a hint from me (as the vmdk > author) to the cluster that this is the vmware virtual hardware > version I have tested this vmdk to run with. It's actually relatively > useful metadata as such. > > In other words, without the patch not a single qemu-img generated vmdk > is 'good' for the system I'm using. Thanks for the explanation, please add this information into the commit message in next submission and also fix the option code so I can ack your patch. Fam
On Wed, Apr 20, 2016 at 8:26 PM, Fam Zheng <famz@redhat.com> wrote: >> In other words, without the patch not a single qemu-img generated vmdk >> is 'good' for the system I'm using. > > Thanks for the explanation, please add this information into the commit message > in next submission and also fix the option code so I can ack your patch. Ack, will do. Thanks
On Wed, 04/20 20:22, Janne Karhunen wrote: > On Wed, Apr 20, 2016 at 7:22 PM, Fam Zheng <famz@redhat.com> wrote: > > > For backward compatibility reason, we shouldn't remove a pre-existing option > > without solid justification and probably a grace period. I suggest adding the > > hwversion option in addition, and document and check it as exclusive with the > > compat6 flag. > > That's certainly doable and kind of makes sense, but I'm not entirely > sure that compat6 flag makes any sense to begin with. Does it? I don't know VMware products well enough to tell, but it has been available since forever, therefore my suggestion is to just keep it which cannot hurt. If we really want to get rid of it, we should start printing a warning message to indicate using the flag is deprecated and remove it one or a few releases later. IMO it's not really worth the effort. Fam
On Wed, Apr 20, 2016 at 9:44 PM, Fam Zheng <famz@redhat.com> wrote: >> That's certainly doable and kind of makes sense, but I'm not entirely >> sure that compat6 flag makes any sense to begin with. Does it? > > I don't know VMware products well enough to tell, but it has been available > since forever, therefore my suggestion is to just keep it which cannot hurt. I sent the new patch yesterday with this option still present and it works the same as it always has. I also tested it to work on all option pairings, so it should be good to go AFAIK. Hope you have some time to check that.
On Fri, 04/22 08:23, Janne Karhunen wrote: > On Wed, Apr 20, 2016 at 9:44 PM, Fam Zheng <famz@redhat.com> wrote: > > >> That's certainly doable and kind of makes sense, but I'm not entirely > >> sure that compat6 flag makes any sense to begin with. Does it? > > > > I don't know VMware products well enough to tell, but it has been available > > since forever, therefore my suggestion is to just keep it which cannot hurt. > > I sent the new patch yesterday with this option still present and it > works the same as it always has. I also tested it to work on all > option pairings, so it should be good to go AFAIK. Hope you have some > time to check that. Thanks for letting me know! I'll take a look soon. (Patches without correctly Cc'ing maintainers are very easy to get neglected. Please remember to use ./scripts/get_maintainer.pl (or ./MAINTAINERS) next time.) Fam
diff --git a/block/vmdk.c b/block/vmdk.c index 45f9d3c..1436785 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1829,8 +1829,8 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) int64_t total_size = 0, filesize; char *adapter_type = NULL; char *backing_file = NULL; + char *hw_version = NULL; char *fmt = NULL; - int flags = 0; int ret = 0; bool flat, split, compress; GString *ext_desc_lines; @@ -1861,7 +1861,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) "# The Disk Data Base\n" "#DDB\n" "\n" - "ddb.virtualHWVersion = \"%d\"\n" + "ddb.virtualHWVersion = \"%s\"\n" "ddb.geometry.cylinders = \"%" PRId64 "\"\n" "ddb.geometry.heads = \"%" PRIu32 "\"\n" "ddb.geometry.sectors = \"63\"\n" @@ -1878,9 +1878,8 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) BDRV_SECTOR_SIZE); adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE); backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE); - if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) { - flags |= BLOCK_FLAG_COMPAT6; - } + hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION); + fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT); if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false)) { zeroed_grain = true; @@ -2001,7 +2000,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) fmt, parent_desc_line, ext_desc_lines->str, - (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4), + hw_version, total_size / (int64_t)(63 * number_heads * BDRV_SECTOR_SIZE), number_heads, @@ -2047,6 +2046,7 @@ exit: } g_free(adapter_type); g_free(backing_file); + g_free(hw_version); g_free(fmt); g_free(desc); g_free(path); @@ -2292,10 +2292,10 @@ static QemuOptsList vmdk_create_opts = { .help = "File name of a base image" }, { - .name = BLOCK_OPT_COMPAT6, - .type = QEMU_OPT_BOOL, - .help = "VMDK version 6 image", - .def_value_str = "off" + .name = BLOCK_OPT_HWVERSION, + .type = QEMU_OPT_STRING, + .help = "VMDK hardware version", + .def_value_str = "4" }, { .name = BLOCK_OPT_SUBFMT, diff --git a/include/block/block_int.h b/include/block/block_int.h index 10d8759..0852316 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -38,12 +38,11 @@ #include "qemu/throttle.h" #define BLOCK_FLAG_ENCRYPT 1 -#define BLOCK_FLAG_COMPAT6 4 #define BLOCK_FLAG_LAZY_REFCOUNTS 8 #define BLOCK_OPT_SIZE "size" #define BLOCK_OPT_ENCRYPT "encryption" -#define BLOCK_OPT_COMPAT6 "compat6" +#define BLOCK_OPT_HWVERSION "hwversion" #define BLOCK_OPT_BACKING_FILE "backing_file" #define BLOCK_OPT_BACKING_FMT "backing_fmt" #define BLOCK_OPT_CLUSTER_SIZE "cluster_size" diff --git a/qemu-doc.texi b/qemu-doc.texi index 79141d3..79f4168 100644 --- a/qemu-doc.texi +++ b/qemu-doc.texi @@ -691,8 +691,8 @@ Supported options: @table @code @item backing_file File name of a base image (see @option{create} subcommand). -@item compat6 -Create a VMDK version 6 image (instead of version 4) +@item hwversion +Specify vmdk hardware version (default 4) @item subformat Specifies which VMDK subformat to use. Valid options are @code{monolithicSparse} (default),