[libvirt] [PATCH] vmware: os x support is broken
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;
On 01/03/2014 10:57 AM, Denis Kondratenko wrote:
Incorrect usage of virAsprintf and vmware-vmx reports to stderr.
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
On Fri, Jan 3, 2014 at 12:14 PM, Eric Blake <eblake@redhat.com> wrote:
On 01/03/2014 10:57 AM, Denis Kondratenko wrote:
Incorrect usage of virAsprintf and vmware-vmx reports to stderr.
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
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@gentoo.org> wrote:
On Fri, Jan 3, 2014 at 12:14 PM, Eric Blake <eblake@redhat.com> wrote:
On 01/03/2014 10:57 AM, Denis Kondratenko wrote:
Incorrect usage of virAsprintf and vmware-vmx reports to stderr.
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
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@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@gentoo.org> wrote:
On Fri, Jan 3, 2014 at 12:14 PM, Eric Blake <eblake@redhat.com> wrote:
On 01/03/2014 10:57 AM, Denis Kondratenko wrote:
Incorrect usage of virAsprintf and vmware-vmx reports to stderr.
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
participants (3)
-
Denis Kondratenko -
Doug Goldstein -
Eric Blake