Re: [libvirt] [PATCH] esx: Fix floppy.fileName handling in the vmx file parser

2013-10-14 Thread Daniel P. Berrange 
On Mon, Oct 14, 2013 at 12:39:09PM -0400, Geoff Hickey wrote:
> The vmx file parsing code was reporting errors when parsing floppy.fileName
> entries if the filename didn't end in .flp. There is no such restriction in
> ESX; even using the GUI to configure floppy filenames you can specify any
> arbitrary file with any extension.
> 
> Fix by changing the vmx parsing code so that it uses the floppy.fileType
> value to determine whether floppy.fileName refers to a block device or a
> regular file.
> 
> Also remove code that would have generated an error if no floppy.fileName
> was specified. This is not an error either.
> ---
>  src/vmx/vmx.c | 28 
>  1 file changed, 4 insertions(+), 24 deletions(-)

Thanks for your contribution. We like to have unit tests to validate
correct handling of XML conversions. Could you add some data files to
validate these particular problems to tests/vmx2xmltest.c

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] esx: Fix floppy.fileName handling in the vmx file parser

2013-10-14 Thread Geoff Hickey 
The vmx file parsing code was reporting errors when parsing floppy.fileName
entries if the filename didn't end in .flp. There is no such restriction in
ESX; even using the GUI to configure floppy filenames you can specify any
arbitrary file with any extension.

Fix by changing the vmx parsing code so that it uses the floppy.fileType
value to determine whether floppy.fileName refers to a block device or a
regular file.

Also remove code that would have generated an error if no floppy.fileName
was specified. This is not an error either.
---
 src/vmx/vmx.c | 28 
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 36bc338..48487f8 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2250,27 +2250,14 @@ virVMXParseDisk(virVMXContext *ctx, 
virDomainXMLOptionPtr xmlopt, virConfPtr con
 goto cleanup;
 }
 } else if (device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
-if (virFileHasSuffix(fileName, ".flp")) {
-if (fileType != NULL) {
-if (STRCASENEQ(fileType, "file")) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Expecting VMX entry '%s' to be 'file' 
but "
- "found '%s'"), fileType_name, fileType);
-goto cleanup;
-}
-}
-
-(*def)->type = VIR_DOMAIN_DISK_TYPE_FILE;
-(*def)->src = ctx->parseFileName(fileName, ctx->opaque);
-
-if ((*def)->src == NULL) {
-goto cleanup;
-}
-} else if (fileType != NULL && STRCASEEQ(fileType, "device")) {
+if (fileType != NULL && STRCASEEQ(fileType, "device")) {
 (*def)->type = VIR_DOMAIN_DISK_TYPE_BLOCK;
 (*def)->src = fileName;
 
 fileName = NULL;
+} else if (fileType != NULL && STRCASEEQ(fileType, "file")) {
+(*def)->type = VIR_DOMAIN_DISK_TYPE_FILE;
+(*def)->src = ctx->parseFileName(fileName, ctx->opaque);
 } else {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid or not yet handled value '%s' "
@@ -3538,13 +3525,6 @@ virVMXFormatFloppy(virVMXContext *ctx, 
virDomainDiskDefPtr def,
 virBufferAsprintf(buffer, "floppy%d.fileType = \"file\"\n", unit);
 
 if (def->src != NULL) {
-if (! virFileHasSuffix(def->src, ".flp")) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Image file for floppy '%s' has unsupported "
- "suffix, expecting '.flp'"), def->dst);
-return -1;
-}
-
 fileName = ctx->formatFileName(def->src, ctx->opaque);
 
 if (fileName == NULL) {
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] esx: Fix floppy.fileName handling in the vmx file parser

2013-10-10 Thread Eric Blake 
On 10/10/2013 02:49 PM, Geoff Hickey wrote:
> Eric: No problem, I'll resubmit with git (sorry about that, it's been long
> enough since my last submit that I forgot about that), but I'll fix the
> issue that Doug pointed out first. As for the pointer comparison to NULL,
> I'm in complete agreement, but that particular source file uses the != NULL
> idiom everywhere. I'd be happy to remove it everywhere, but that should be
> done as a separate submit, I would think?

Indeed, if you're going to clean up the entire file, do it as a separate
commit.

> 
> Thanks, 2nd try soon. :)

Looking forward to it.

Also, we tend to avoid top-posting on this list.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] esx: Fix floppy.fileName handling in the vmx file parser

2013-10-10 Thread Geoff Hickey 
Eric: No problem, I'll resubmit with git (sorry about that, it's been long
enough since my last submit that I forgot about that), but I'll fix the
issue that Doug pointed out first. As for the pointer comparison to NULL,
I'm in complete agreement, but that particular source file uses the != NULL
idiom everywhere. I'd be happy to remove it everywhere, but that should be
done as a separate submit, I would think?

Thanks, 2nd try soon. :)


On Thu, Oct 10, 2013 at 4:15 PM, Doug Goldstein  wrote:

> On Thu, Oct 10, 2013 at 2:21 PM, Eric Blake  wrote:
> > On 10/09/2013 07:39 PM, Geoff Hickey wrote:
> >> The vmx file parsing code was reporting errors when parsing
> floppy.fileName
> >> entries if the filename didn't end in .flp. There is no such
> restriction in
> >> ESX; even using the GUI to configure floppy filenames you can specify
> any
> >> arbitrary file with any extension.
> >>
> >> Fix by changing the vmx parsing code so that it uses the floppy.fileType
> >> value to determine whether floppy.fileName refers to a block device or a
> >> regular file.
> >> ---
> >>  src/vmx/vmx.c | 26 +-
> >>  1 file changed, 5 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> >> index 38b7cc0..ffe7e7a 100644
> >> --- a/src/vmx/vmx.c
> >> +++ b/src/vmx/vmx.c
> >> @@ -2250,27 +2250,18 @@ virVMXParseDisk(virVMXContext *ctx,
> >> virDomainXMLOptionPtr xmlopt, virConfPtr con
> >>  goto cleanup;
> >>  }
> >>  } else if (device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> >> -if (virFileHasSuffix(fileName, ".flp")) {
> >> -if (fileType != NULL) {
> >> -if (STRCASENEQ(fileType, "file")) {
> >> -virReportError(VIR_ERR_INTERNAL_ERROR,
> >> -   _("Expecting VMX entry '%s' to be
> >> 'file' but "
> >
> > Your mailer corrupted your patch.  Would you mind resending it via 'git
> > send-email', or at a bare minimum as an attachment rather than inline,
> > so that maintainers can just 'git am' it?
> >
> > Applying: esx: Fix floppy.fileName handling in the vmx file parser
> > fatal: corrupt patch at line 10
> > Repository lacks necessary blobs to fall back on 3-way merge.
> > Cannot fall back to three-way merge.
> > Patch failed at 0001 esx: Fix floppy.fileName handling in the vmx file
> > parser
> >
> > See also http://libvirt.org/hacking.html for hints.
> >
> >> +if (fileType != NULL && STRCASEEQ(fileType, "device")) {
> >
> > It's simpler to just use a pointer variable in boolean context instead
> > of explicit comparison against NULL (this isn't Java).  As in:
> >
> > if (fileType && STRCASEEQ(fileType, "device")) {
> >
> > At any rate, your change looks reasonable to me; I wouldn't mind
> > applying it if you can resend to touch up those issues.
> >
> > --
> > Eric Blake   eblake redhat com+1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> >
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>
> FWIW, I've been fixing a number of these poor assumptions lately.
> There's no limitation that there must be a fileName specified either
> and the way the patch is formulated it will treat it as a fatal error
> that there is a floppy drive defined with no file name for the floppy
> image.
>
> --
> Doug Goldstein
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] esx: Fix floppy.fileName handling in the vmx file parser

2013-10-10 Thread Doug Goldstein 
On Thu, Oct 10, 2013 at 2:21 PM, Eric Blake  wrote:
> On 10/09/2013 07:39 PM, Geoff Hickey wrote:
>> The vmx file parsing code was reporting errors when parsing floppy.fileName
>> entries if the filename didn't end in .flp. There is no such restriction in
>> ESX; even using the GUI to configure floppy filenames you can specify any
>> arbitrary file with any extension.
>>
>> Fix by changing the vmx parsing code so that it uses the floppy.fileType
>> value to determine whether floppy.fileName refers to a block device or a
>> regular file.
>> ---
>>  src/vmx/vmx.c | 26 +-
>>  1 file changed, 5 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
>> index 38b7cc0..ffe7e7a 100644
>> --- a/src/vmx/vmx.c
>> +++ b/src/vmx/vmx.c
>> @@ -2250,27 +2250,18 @@ virVMXParseDisk(virVMXContext *ctx,
>> virDomainXMLOptionPtr xmlopt, virConfPtr con
>>  goto cleanup;
>>  }
>>  } else if (device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
>> -if (virFileHasSuffix(fileName, ".flp")) {
>> -if (fileType != NULL) {
>> -if (STRCASENEQ(fileType, "file")) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>> -   _("Expecting VMX entry '%s' to be
>> 'file' but "
>
> Your mailer corrupted your patch.  Would you mind resending it via 'git
> send-email', or at a bare minimum as an attachment rather than inline,
> so that maintainers can just 'git am' it?
>
> Applying: esx: Fix floppy.fileName handling in the vmx file parser
> fatal: corrupt patch at line 10
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 esx: Fix floppy.fileName handling in the vmx file
> parser
>
> See also http://libvirt.org/hacking.html for hints.
>
>> +if (fileType != NULL && STRCASEEQ(fileType, "device")) {
>
> It's simpler to just use a pointer variable in boolean context instead
> of explicit comparison against NULL (this isn't Java).  As in:
>
> if (fileType && STRCASEEQ(fileType, "device")) {
>
> At any rate, your change looks reasonable to me; I wouldn't mind
> applying it if you can resend to touch up those issues.
>
> --
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

FWIW, I've been fixing a number of these poor assumptions lately.
There's no limitation that there must be a fileName specified either
and the way the patch is formulated it will treat it as a fatal error
that there is a floppy drive defined with no file name for the floppy
image.

-- 
Doug Goldstein

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] esx: Fix floppy.fileName handling in the vmx file parser

2013-10-10 Thread Eric Blake 
On 10/09/2013 07:39 PM, Geoff Hickey wrote:
> The vmx file parsing code was reporting errors when parsing floppy.fileName
> entries if the filename didn't end in .flp. There is no such restriction in
> ESX; even using the GUI to configure floppy filenames you can specify any
> arbitrary file with any extension.
> 
> Fix by changing the vmx parsing code so that it uses the floppy.fileType
> value to determine whether floppy.fileName refers to a block device or a
> regular file.
> ---
>  src/vmx/vmx.c | 26 +-
>  1 file changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 38b7cc0..ffe7e7a 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -2250,27 +2250,18 @@ virVMXParseDisk(virVMXContext *ctx,
> virDomainXMLOptionPtr xmlopt, virConfPtr con
>  goto cleanup;
>  }
>  } else if (device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> -if (virFileHasSuffix(fileName, ".flp")) {
> -if (fileType != NULL) {
> -if (STRCASENEQ(fileType, "file")) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("Expecting VMX entry '%s' to be
> 'file' but "

Your mailer corrupted your patch.  Would you mind resending it via 'git
send-email', or at a bare minimum as an attachment rather than inline,
so that maintainers can just 'git am' it?

Applying: esx: Fix floppy.fileName handling in the vmx file parser
fatal: corrupt patch at line 10
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 esx: Fix floppy.fileName handling in the vmx file
parser

See also http://libvirt.org/hacking.html for hints.

> +if (fileType != NULL && STRCASEEQ(fileType, "device")) {

It's simpler to just use a pointer variable in boolean context instead
of explicit comparison against NULL (this isn't Java).  As in:

if (fileType && STRCASEEQ(fileType, "device")) {

At any rate, your change looks reasonable to me; I wouldn't mind
applying it if you can resend to touch up those issues.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] esx: Fix floppy.fileName handling in the vmx file parser

2013-10-09 Thread Geoff Hickey 
The vmx file parsing code was reporting errors when parsing floppy.fileName
entries if the filename didn't end in .flp. There is no such restriction in
ESX; even using the GUI to configure floppy filenames you can specify any
arbitrary file with any extension.

Fix by changing the vmx parsing code so that it uses the floppy.fileType
value to determine whether floppy.fileName refers to a block device or a
regular file.
---
 src/vmx/vmx.c | 26 +-
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 38b7cc0..ffe7e7a 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2250,27 +2250,18 @@ virVMXParseDisk(virVMXContext *ctx,
virDomainXMLOptionPtr xmlopt, virConfPtr con
 goto cleanup;
 }
 } else if (device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
-if (virFileHasSuffix(fileName, ".flp")) {
-if (fileType != NULL) {
-if (STRCASENEQ(fileType, "file")) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Expecting VMX entry '%s' to be
'file' but "
- "found '%s'"), fileType_name,
fileType);
-goto cleanup;
-}
-}
+if (fileType != NULL && STRCASEEQ(fileType, "device")) {
+(*def)->type = VIR_DOMAIN_DISK_TYPE_BLOCK;
+(*def)->src = fileName;

+fileName = NULL;
+} else if (fileType != NULL && STRCASEEQ(fileType, "file")) {
 (*def)->type = VIR_DOMAIN_DISK_TYPE_FILE;
 (*def)->src = ctx->parseFileName(fileName, ctx->opaque);

 if ((*def)->src == NULL) {
 goto cleanup;
 }
-} else if (fileType != NULL && STRCASEEQ(fileType, "device")) {
-(*def)->type = VIR_DOMAIN_DISK_TYPE_BLOCK;
-(*def)->src = fileName;
-
-fileName = NULL;
 } else {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid or not yet handled value '%s' "
@@ -3538,13 +3529,6 @@ virVMXFormatFloppy(virVMXContext *ctx,
virDomainDiskDefPtr def,
 virBufferAsprintf(buffer, "floppy%d.fileType = \"file\"\n", unit);

 if (def->src != NULL) {
-if (! virFileHasSuffix(def->src, ".flp")) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Image file for floppy '%s' has
unsupported "
- "suffix, expecting '.flp'"), def->dst);
-return -1;
-}
-
 fileName = ctx->formatFileName(def->src, ctx->opaque);

 if (fileName == NULL) {
-- 
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list