11:57 a.m.
Incorrect usage of virAsprintf and vmware-vmx reports to stderr.
--- https://bugzilla.redhat.com/show_bug.cgi?id=1036248
diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c
index 027e245..477dc72 100644
--- a/src/vmware/vmware_conf.c
+++ b/src/vmware/vmware_conf.c
@@ -271,17 +271,17 @@ vmwareExtractVersion(struct vmware_driver *driver)
switch (driver->type) {
case VMWARE_DRIVER_PLAYER:
- if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer"))
+ if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer") < 0)
goto cleanup;
break;
case VMWARE_DRIVER_WORKSTATION:
- if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmware"))
+ if (virAsprintf(&bin, "%s/%s", vmwarePath, "vmware")
< 0)
goto cleanup;
break;
case VMWARE_DRIVER_FUSION:
- if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmware-vmx"))
+ if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmware-vmx") < 0)
goto cleanup;
break;
@@ -293,6 +293,9 @@ vmwareExtractVersion(struct vmware_driver *driver)
cmd = virCommandNewArgList(bin, "-v", NULL);
virCommandSetOutputBuffer(cmd, &outbuf);
+
+ // OS X 10.9.1 and some earlier ver: vmware-vmx reports ver to stderr
+ virCommandSetErrorBuffer(cmd, &outbuf);
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
12:14 p.m.
On 01/03/2014 10:57 AM, Denis Kondratenko wrote:
Incorrect usage of virAsprintf and vmware-vmx reports to stderr.
--- https://bugzilla.redhat.com/show_bug.cgi?id=1036248
Oh my - the triple --- leadin confused 'git am':
Applying: vmware: os x support is broken
fatal: patch fragment without header at line 42: @@ -271,17 +271,17 @@
vmwareExtractVersion(struct vmware_driver
*driver)</div><div> </div><div> switch (driver->type)
{</div><div> case VMWARE_DRIVER_PLAYER:</div><div>-
if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer"))</div>
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 vmware: os x support is broken
so I had to apply it by hand.
case VMWARE_DRIVER_PLAYER:
- if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer"))
+ if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer") < 0)
Definitely correct!
cmd = virCommandNewArgList(bin, "-v", NULL);
virCommandSetOutputBuffer(cmd, &outbuf);
+
+ // OS X 10.9.1 and some earlier ver: vmware-vmx reports ver to stderr
We prefer to avoid C99 comments, and the code is self-explanatory enough
that it was easier to just drop the comment (and put it in the commit
message instead).
ACK and pushed. Congrats on your first libvirt patch!
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:34 p.m.
On Fri, Jan 3, 2014 at 12:14 PM, Eric Blake <eblake(a)redhat.com> wrote:
On 01/03/2014 10:57 AM, Denis Kondratenko wrote:
> Incorrect usage of virAsprintf and vmware-vmx reports to stderr.
>
> --- https://bugzilla.redhat.com/show_bug.cgi?id=1036248
Oh my - the triple --- leadin confused 'git am':
Applying: vmware: os x support is broken
fatal: patch fragment without header at line 42: @@ -271,17 +271,17 @@
vmwareExtractVersion(struct vmware_driver
*driver)</div><div> </div><div> switch (driver->type)
{</div><div> case VMWARE_DRIVER_PLAYER:</div><div>-
if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer"))</div>
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 vmware: os x support is broken
so I had to apply it by hand.
> case VMWARE_DRIVER_PLAYER:
> - if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer"))
> + if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer") < 0)
Definitely correct!
> cmd = virCommandNewArgList(bin, "-v", NULL);
> virCommandSetOutputBuffer(cmd, &outbuf);
> +
> + // OS X 10.9.1 and some earlier ver: vmware-vmx reports ver to stderr
We prefer to avoid C99 comments, and the code is self-explanatory enough
that it was easier to just drop the comment (and put it in the commit
message instead).
Sadly the comment actually doesn't make any sense. vmware-vmx is a
program shipped by VMware and doesn't select stdout or stderr based on
OS X version. It selects it based on whether the arguments supplied
are valid or not for the command. If they aren't valid then its stderr
(like most UNIX utilities) and if they're valid it goes via stdout. I
believe we had to use a bad command (and thus stderr) on OS X always
to get the version info as nothing that supplied the version returned
a successful.
Denis,
I don't own a copy of VMware Fusion (or any VMware product) so I can't
personally test. I have a number of branches which have some potential
fixes (I believe 1 series has been posted RFC without a response) but
don't at the moment have anyone to test them. So if you're interested
in testing them I can provide you with an alternate brew install
command and we can maybe land some other fixes.
--
Doug Goldstein
12:17 a.m.
NP, I could test something for you. Send me a patch or whole brew receipt.
On Jan 4, 2014 7:34 AM, "Doug Goldstein" <cardoe(a)gentoo.org> wrote:
On Fri, Jan 3, 2014 at 12:14 PM, Eric Blake <eblake(a)redhat.com>
wrote:
> On 01/03/2014 10:57 AM, Denis Kondratenko wrote:
>> Incorrect usage of virAsprintf and vmware-vmx reports to stderr.
>>
>> --- https://bugzilla.redhat.com/show_bug.cgi?id=1036248
>
> Oh my - the triple --- leadin confused 'git am':
>
> Applying: vmware: os x support is broken
> fatal: patch fragment without header at line 42: @@ -271,17 +271,17 @@
> vmwareExtractVersion(struct vmware_driver
> *driver)</div><div> </div><div> switch
(driver->type)
> {</div><div> case VMWARE_DRIVER_PLAYER:</div><div>-
> if (virAsprintf(&bin, "%s/%s", vmwarePath,
> "vmplayer"))</div>
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 vmware: os x support is broken
>
> so I had to apply it by hand.
>
>
>> case VMWARE_DRIVER_PLAYER:
>> - if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer"))
>> + if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer") < 0)
>
> Definitely correct!
>
>
>> cmd = virCommandNewArgList(bin, "-v", NULL);
>> virCommandSetOutputBuffer(cmd, &outbuf);
>> +
>> + // OS X 10.9.1 and some earlier ver: vmware-vmx reports ver to
stderr
>
> We prefer to avoid C99 comments, and the code is self-explanatory enough
> that it was easier to just drop the comment (and put it in the commit
> message instead).
Sadly the comment actually doesn't make any sense. vmware-vmx is a
program shipped by VMware and doesn't select stdout or stderr based on
OS X version. It selects it based on whether the arguments supplied
are valid or not for the command. If they aren't valid then its stderr
(like most UNIX utilities) and if they're valid it goes via stdout. I
believe we had to use a bad command (and thus stderr) on OS X always
to get the version info as nothing that supplied the version returned
a successful.
Denis,
I don't own a copy of VMware Fusion (or any VMware product) so I can't
personally test. I have a number of branches which have some potential
fixes (I believe 1 series has been posted RFC without a response) but
don't at the moment have anyone to test them. So if you're interested
in testing them I can provide you with an alternate brew install
command and we can maybe land some other fixes.
--
Doug Goldstein
2 a.m.
Behaviour of vmware* utilities is hidden from us.
It seems to me that they didn't expect vmware-vmx direct usage so
reporting everything to stderr in case of os x.
This obvious option is reported to stderr too and has some bug :
$ /Applications/VMware\ Fusion.app/Contents/Library/vmware-vmx -?
/Applications/VMware Fusion.app/Contents/Library/vmware-vmx: illegal option -- ?
To run the user interface, use vmware and not vmware-vmx.
Usage: vmware-vmx [<flags>] [configfile]
where <flags> are:
-P start paused
-q exit at power off
-s name=value set variable NAME to VALUE
-x power on when program starts
-X as -x, also go to full screen mode
-v print program version
-# Product-specific settings
-e name extract a resource specified by name
-? Usage
-@ VMDB connection to UI
vmware-vmx: this executable should not be invoked directly.
Cannot start vmware-vmx.
-3 Failed to parse command line options.
"vmware-vmx" -h probably was chosen as only one that reports version
string (btw it is strange that version is not saved to driver, it just
left in local var).
So I think connecting stderr is not bad at all, as it shouldn't change
behaviour and will address this strange VMware bug.
On Sat, Jan 4, 2014 at 8:17 AM, Denis Kondratenko
<denis.kondratenko(a)gmail.com> wrote:
NP, I could test something for you. Send me a patch or whole brew receipt.
On Jan 4, 2014 7:34 AM, "Doug Goldstein" <cardoe(a)gentoo.org> wrote:
>
> On Fri, Jan 3, 2014 at 12:14 PM, Eric Blake <eblake(a)redhat.com> wrote:
> > On 01/03/2014 10:57 AM, Denis Kondratenko wrote:
> >> Incorrect usage of virAsprintf and vmware-vmx reports to stderr.
> >>
> >> --- https://bugzilla.redhat.com/show_bug.cgi?id=1036248
> >
> > Oh my - the triple --- leadin confused 'git am':
> >
> > Applying: vmware: os x support is broken
> > fatal: patch fragment without header at line 42: @@ -271,17 +271,17 @@
> > vmwareExtractVersion(struct vmware_driver
> > *driver)</div><div> </div><div> switch
(driver->type)
> > {</div><div> case
VMWARE_DRIVER_PLAYER:</div><div>-
> > if (virAsprintf(&bin, "%s/%s", vmwarePath,
> > "vmplayer"))</div>
> > Repository lacks necessary blobs to fall back on 3-way merge.
> > Cannot fall back to three-way merge.
> > Patch failed at 0001 vmware: os x support is broken
> >
> > so I had to apply it by hand.
> >
> >
> >> case VMWARE_DRIVER_PLAYER:
> >> - if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer"))
> >> + if (virAsprintf(&bin, "%s/%s", vmwarePath,
"vmplayer") < 0)
> >
> > Definitely correct!
> >
> >
> >> cmd = virCommandNewArgList(bin, "-v", NULL);
> >> virCommandSetOutputBuffer(cmd, &outbuf);
> >> +
> >> + // OS X 10.9.1 and some earlier ver: vmware-vmx reports ver to stderr
> >
> > We prefer to avoid C99 comments, and the code is self-explanatory enough
> > that it was easier to just drop the comment (and put it in the commit
> > message instead).
>
> Sadly the comment actually doesn't make any sense. vmware-vmx is a
> program shipped by VMware and doesn't select stdout or stderr based on
> OS X version. It selects it based on whether the arguments supplied
> are valid or not for the command. If they aren't valid then its stderr
> (like most UNIX utilities) and if they're valid it goes via stdout. I
> believe we had to use a bad command (and thus stderr) on OS X always
> to get the version info as nothing that supplied the version returned
> a successful.
>
> Denis,
>
> I don't own a copy of VMware Fusion (or any VMware product) so I can't
> personally test. I have a number of branches which have some potential
> fixes (I believe 1 series has been posted RFC without a response) but
> don't at the moment have anyone to test them. So if you're interested
> in testing them I can provide you with an alternate brew install
> command and we can maybe land some other fixes.
>
> --
> Doug Goldstein
3976
days inactive
3977
days old
4 comments
3 participants
participants (3)
-
Denis Kondratenko -
Doug Goldstein -
Eric Blake