[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-221861017 Can I get one more review on this one? I think we are basically set otherwise. Thx... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-221780994 @swill Resolved the merge conflicts. They were because of import statements. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-221763988 This one needs some work. We have merge conflicts and we need another code review. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r64009630
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -594,6 +597,25 @@ private PlugNicAnswer execute(final PlugNicCommand
cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
--- End diff --
The message can be more specific and explanatory. The SshHelper shouldn't
throw such a generic exception anyway. This is sloppy development and we now
have a good chance to improve on it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r6362
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -594,6 +597,25 @@ private PlugNicAnswer execute(final PlugNicCommand
cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
--- End diff --
I don't understand one point what difference it would make to catch
specific actions and take same action in all case then catching generic
exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63997069
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -594,6 +597,25 @@ private PlugNicAnswer execute(final PlugNicCommand
cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
--- End diff --
I understand you are basing yourself on bad code. That does not make it a
good practice. Either the SshHelper should be refactored to some extend and the
possible IOException should be caught seperately. See my general comment to
this PR, please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63988259
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
--- End diff --
Answered above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63988285
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -1298,6 +1304,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+VmwareManager mgr =
getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+File keyFile = mgr.getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
--- End diff --
Answered above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63988200
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -594,6 +597,25 @@ private PlugNicAnswer execute(final PlugNicCommand
cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
--- End diff --
SshHelper is throwing Exception so I am catching Exception. If you are
talking about handling it here and not passing to CopyFileInVmAnswer then
CopyFileInVmAnswer is just passing the exception message and not full
exception. Applicable constructor from CopyFileInVmAnswer
public CopyFileInVmAnswer(CopyFileInVmCommand cmd, Exception e) {
super(cmd, false, e.getMessage());
}
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-220464828 Really like the functionality so I think we must get this in. I have some remarks about the code that I stand by even though they are not entirely to blame on @anshul1886 as he calls methods fthrowing Exception, rom SshHelper mainly. It is a bad smell due to the older code and will not facilitate trouble shooting for the end user. So I would like to see the exception-handlers be enriched with some better diagnosis and *maybe* messages. nothing else to nag about. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63961096
--- Diff:
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.xenserver.resource.wrapper.xenbase;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.xenserver.resource.CitrixResourceBase;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ExecutionResult;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class CitrixCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(CitrixCopyFileInVmCommandWrapper.class);
+
+@Override
+public Answer execute(final CopyFileInVmCommand cmd, final
CitrixResourceBase citrixResourceBase) {
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for(File f : FileUtils.listFiles(file,
TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ExecutionResult result =
citrixResourceBase.copyFileInVm(cmd.getVmIp(), f, cmd.getDest());
+if(!result.isSuccess()) {
+return new CopyFileInVmAnswer(cmd,
result.getDetails());
+}
+}
+} else {
+ExecutionResult result =
citrixResourceBase.copyFileInVm(cmd.getVmIp(), file, cmd.getDest());
+if(!result.isSuccess()) {
+return new CopyFileInVmAnswer(cmd,
result.getDetails());
+}
+}
+}
+
+} catch (Exception e) {
--- End diff --
please catch more specific exceptions
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63960701
--- Diff:
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java
---
@@ -5402,4 +5402,20 @@ public boolean
attachConfigDriveToMigratedVm(Connection conn, String vmName, Str
}
-}
+public ExecutionResult copyFileInVm(String vmIp, File file, String
dest) {
+final Connection conn = getConnection();
+final String hostPath = "/tmp/";
+
+s_logger.debug("Copying file to VM with ip " + vmIp + " using host
" + _host.getIp());
+try {
+SshHelper.scpTo(_host.getIp(), 22, _username, null,
_password.peek(), hostPath, file.getCanonicalPath(), null);
+} catch (final Exception e) {
--- End diff --
please catch more specific exceptions
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63960622
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -1298,6 +1304,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+VmwareManager mgr =
getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+File keyFile = mgr.getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
--- End diff --
please catch more specific exception(s)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63959786
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
--- End diff --
please replace the two lines above with
```try (File file = new File(cmd.getSrc());) {```
to make sure the file resource is closed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63959351
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -594,6 +597,25 @@ private PlugNicAnswer execute(final PlugNicCommand
cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
--- End diff --
can you catch more specific exceptions here, please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63959496
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
--- End diff --
please, catch more specific exceptions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-220447959 I think with @milamberspace's review, we are only missing one code review to get this one in. Thanks everyone... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user milamberspace commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-220446031 LGTM. Tested manually with a real test deployment of CS49, with my French azerty keyboard: works fine. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-220025191 This shows that it does not break anything, but I don't think we have any verification that this feature works as expected yet... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-220024998 ### CI RESULTS ``` Tests Run: 85 Skipped: 0 Failed: 1 Errors: 0 Duration: 4h 18m 27s ``` **Summary of the problem(s):** ``` FAIL: Test redundant router internals -- Traceback (most recent call last): File "/data/git/cs2/cloudstack/test/integration/smoke/test_routers_network_ops.py", line 842, in test_01_isolate_network_FW_PF_default_routes_egress_true "Attempt to retrieve google.com index page should be successful!" AssertionError: Attempt to retrieve google.com index page should be successful! -- Additional details in: /tmp/MarvinLogs/test_network_A6H565/results.txt ``` **Associated Uploads** **`/tmp/MarvinLogs/DeployDataCenter__May_18_2016_08_23_35_7BOYAF:`** * [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/DeployDataCenter__May_18_2016_08_23_35_7BOYAF/dc_entries.obj) * [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/DeployDataCenter__May_18_2016_08_23_35_7BOYAF/failed_plus_exceptions.txt) * [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/DeployDataCenter__May_18_2016_08_23_35_7BOYAF/runinfo.txt) **`/tmp/MarvinLogs/test_network_A6H565:`** * [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_network_A6H565/failed_plus_exceptions.txt) * [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_network_A6H565/results.txt) * [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_network_A6H565/runinfo.txt) **`/tmp/MarvinLogs/test_vpc_routers_GFBFLL:`** * [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_vpc_routers_GFBFLL/failed_plus_exceptions.txt) * [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_vpc_routers_GFBFLL/results.txt) * [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_vpc_routers_GFBFLL/runinfo.txt) Uploads will be available until `2016-07-18 02:00:00 +0200 CEST` *Comment created by [`upr comment`](https://github.com/cloudops/upr).* --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
GitHub user anshul1886 reopened a pull request: https://github.com/apache/cloudstack/pull/669 Made the adding new keyboard language support easier https://issues.apache.org/jira/browse/CLOUDSTACK-8665 This branch has implemented following improvements in console proxy keyboard language support 1) ajaxviewer.js and ajaxkeys.js are main files involved in key code translations. These files now can be copied in systemvm/js folder and they will be copied to CPVM with stop/start performed on it. 2) Started passing parameters to CPVM needed to resolve the ambiguous cases of keycode translations. 3) Generalise the framework such that one needs to modify only ajaxkeys.js (file which has keycode mappings) without need of much knowledge in js. FS for this feature is available at https://cwiki.apache.org/confluence/display/CLOUDSTACK/Support+for+non-US+keyboards+in+Console+Proxy After these changes how to add keyboard support for new language or fix existing broken keys WIP document is available at https://cwiki.apache.org/confluence/display/CLOUDSTACK/Adding+support+for+non-US+Keyboard+for+Console+Proxy You can merge this pull request into a Git repository by running: $ git pull https://github.com/anshul1886/cloudstack-1 nonuskeyboardsupport Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/669.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #669 commit 715f18a1f26b7cccea8a8221a6cafee14e91f3a3 Author: Anshul Gangwar Date: 2015-08-04T04:54:16Z CLOUDSTACK-8665: Added following improvements: 1. Added support for copying js files from management server to console proxy VM with stop start 2. Generalise console keyboard support framework 3. Passing additional parameters which will be needed for keyboard mappings for vm console 4. Moved the console Keyboard Options to new file so that user can add keyboard options easily 5. Improved memory footprint, now keeping only required locale mappings 6. Added more conditions while setting up translation table 7. Improved browser detection for keyboard mappings 8. Formatted ajaxviewer.js and ajaxkeys.js with spaces instead of tabs --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 closed the pull request at: https://github.com/apache/cloudstack/pull/669 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-219918785 @swill They are failing on a test which don't even exist in source code. That test is in PR #1360 which is not yet merged so looks like some cleanup issue. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-219793270 @anshul1886 can you review the Jenkin errors and close and reopen the PR if they do not seem relevant to kick off the Jenkins run again. Thx... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-219672865 @milamberspace Added the localisation key in message.properties and now using it in consoleKeyboardOptions --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user milamberspace commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/669#discussion_r63469835 --- Diff: ui/consoleKeyboardOptions.jsp --- @@ -0,0 +1,24 @@ +<%-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + --%> + + + + + +French AZERTY keyboard --- End diff -- That will be better if the French label use a localization key from messages.properties like the other languages. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-219620496 @swill Rebased against the current master and also squashed the commits into one. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-219129109 Thanks @anshul1886. Keep in mind that the freeze date for 4.9 is Monday next week. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-218980649 @swill Its showing lot of conflicts currently and will take some time to resolve those. Will update PR once I will get some time. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-218869670 This PR currently have merge conflicts. Can you rebase please? Thx... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user rhtyd commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-216186163 @anshul1886 please rebase against latest master and share status of your PR This looks like an interesting PR we should have --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-175664522 @sedukull @anshul1886 please rebase against latest branch and help re-review --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on the pull request: https://github.com/apache/cloudstack/pull/669#issuecomment-131683677 @sedukull , Can you give a LGTM now? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/669#discussion_r36952117 --- Diff: client/pom.xml --- @@ -478,6 +478,12 @@ + --- End diff -- Its good that it does not say that UT are meant to test "corner" cases or "some uncertainity", as per your earlier understanding. It talks about TDD, in which UT are not meant to be written post the code, its good to follow TDD if we can write tests before the code. But if we missed it, and If we feel there is a value add in writing some tests, which can benefit others and helps testing the code in long run, its appreciated to add few, there is no harm. We could see many commits coming are having UT recently. Some even made sure to get near 100% coverage for their code. We know its not a new distinct module, its up to you and whatever the community decides!!! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/669#discussion_r36949815 --- Diff: client/pom.xml --- @@ -478,6 +478,12 @@ + --- End diff -- My understanding about unit testing is similar to mentioned in this blog http://simpleprogrammer.com/2010/10/15/the-purpose-of-unit-testing/ --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/669#discussion_r36839282 --- Diff: client/pom.xml --- @@ -478,6 +478,12 @@ + --- End diff -- Thanks for the FS link, it was helpful. Regarding the unit test, i would disagree to say that they are meant to test corner cases. Not sure, what it doesnt make sense to you. In general, unit test purpose is not what you meant, in this commit, if we see we modified near to 28 code files, added new classes and code, it makes sense to have unit tests for the added code. It will help to test the code units and individual functional blocks during build, these areas not always are touched by functional tests. It adds to the coverage, make sure code modified down the lane adheres to the contract through failures in tests etc. Its appreciated, if we can have few unit tests added to this, right now they are zero tests for this change. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/669#discussion_r36833745 --- Diff: client/pom.xml --- @@ -478,6 +478,12 @@ + --- End diff -- FS https://cwiki.apache.org/confluence/display/CLOUDSTACK/Support+for+non-US+keyboards+in+Console+Proxy has details regarding console proxy workflow which is present in description. In developer setup these files are always present so adding unit test for certain (which will always pass) thing doesn't make sense to me. In my opinion unit test are meant to test corner cases or some uncertainty. I will try to improve description. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/669#discussion_r36832023 --- Diff: client/pom.xml --- @@ -478,6 +478,12 @@ + --- End diff -- Do we have any unit-tests added for this change? As well, if we possible, can we have a document for the console proxy workflow available on wiki ? This may help people to know the exact workflow or class interactions with business flow. As well, a small change to this request message "Made the adding new keyboard language support easier" could be beneficial i believe, helps people to know the complete context for this change. Note :this is not specific to this line, but a generic note. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36711314
--- Diff: systemvm/js/ajaxviewer.js ---
@@ -184,7 +205,7 @@ KeyboardMapper.prototype = {
if(eventType == AjaxViewer.KEY_UP && (code ==
AjaxViewer.JS_KEY_ALT || code == AjaxViewer.JS_KEY_CTRL))
this.mappedInput.push({type : eventType, code:
this.jsX11KeysymMap[code], modifiers: modifiers});
- } else if(eventType == AjaxViewer.KEY_PRESS && guestos ==
'null') {
+ } else if(eventType == AjaxViewer.KEY_PRESS &&
guestos.toLowerCase() != 'windows') {
--- End diff --
Plan is to remove all OS strings and don't use them specifically at all.
I have left it here like this as removing this may break existing key
mappings. Getting key mappings right is time consuming and this code was
written to get them right.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r3676
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands
cmds, VirtualMachineProfile prof
CheckSshCommand check = new
CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+try {
+File uiFiles = new File("systemvm/js");
+if (!uiFiles.exists()) {
+uiFiles = new
File("/usr/share/cloudstack-common/systemvm/js");
+}
+if (uiFiles.exists() && uiFiles.isDirectory()) {
+CopyFileInVmCommand copyFile = new
CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js",
controlNic.getIp4Address());
+cmds.addCommand("copyFile", copyFile);
+} else {
+s_logger.error("Couldn't locate localization files for
console proxy");
+return false;
+}
+} catch (IOException e) {
+s_logger.error("Failed to copy localization files for console
proxy: " + e.getMessage());
--- End diff --
I think this case is also similar to else case and should be considered
like that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36710980
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands
cmds, VirtualMachineProfile prof
CheckSshCommand check = new
CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+try {
+File uiFiles = new File("systemvm/js");
+if (!uiFiles.exists()) {
+uiFiles = new
File("/usr/share/cloudstack-common/systemvm/js");
+}
+if (uiFiles.exists() && uiFiles.isDirectory()) {
+CopyFileInVmCommand copyFile = new
CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js",
controlNic.getIp4Address());
+cmds.addCommand("copyFile", copyFile);
+} else {
+s_logger.error("Couldn't locate localization files for
console proxy");
+return false;
+}
+} catch (IOException e) {
+s_logger.error("Failed to copy localization files for console
proxy: " + e.getMessage());
--- End diff --
I think above answer is still valid for your concern.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36710950
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
+s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM "
+ cmd.getVmIp(), e);
+return new CopyFileInVmAnswer(cmd, e);
+}
+return new CopyFileInVmAnswer(cmd);
--- End diff --
That line cannot through exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36710924
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
--- End diff --
This is written mainly for copying js files to CPVM. And those js needs to
be present on cloudstack installation. If somebody is able to manipulate
CloudStack installation machine then that is major issue and we can't do
anything about that. This makes me to believe that it doesn't makes much sense
to do some file validation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36710619
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
--- End diff --
This is hypervisor specific code and each hypervisor is achieving it
differently. To me it doesn't make sense to put it in lib or something like
that as it will unnecessarily add complexity without achieving much.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36705867
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
--- End diff --
My note was related to the below example.
example_dir_for_iteration contains say 5 entries as follows (dira,
text_file_a,socket_file_a,block_device_file_a,zip_file_a), in else block of the
code, we are copying all files. Now, in your case where you explicitly
mentioned key file path(/root/..) these file types may not be possible, but
these type of copying without file types checks, leads to vulnerabilities in
the application long run. So, i mentioned to see we are copying files as
mentioned in else block, do we want to add some file type check that we are
interesting in copying, for that i gave an example where directory contains
multiple file types.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36705320
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
--- End diff --
I mean the below code is used across few code files in same form or some
modified form, thats where i made a note, to avoid duplication. If its possible
to make one common definition and use across.
File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for(File f : FileUtils.listFiles(file,
TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ExecutionResult result =
citrixResourceBase.copyFileInVm(cmd.getVmIp(), f, cmd.getDest());
+if(!result.isSuccess()) {
+return new CopyFileInVmAnswer(cmd,
result.getDetails());
+}
+}
+} else {
+ExecutionResult result =
citrixResourceBase.copyFileInVm(cmd.getVmIp(), file, cmd.getDest());
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36705202
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
+s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM "
+ cmd.getVmIp(), e);
+return new CopyFileInVmAnswer(cmd, e);
+}
+return new CopyFileInVmAnswer(cmd);
--- End diff --
Its not a big issue as such, but iam seeing like if there is an exception
at liine 617: "return new CopyFileIn.. (cmd)", then catch will still throw
your new object created. Again, its not a great issue to be worry about!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36705033
--- Diff: systemvm/js/ajaxviewer.js ---
@@ -184,7 +205,7 @@ KeyboardMapper.prototype = {
if(eventType == AjaxViewer.KEY_UP && (code ==
AjaxViewer.JS_KEY_ALT || code == AjaxViewer.JS_KEY_CTRL))
this.mappedInput.push({type : eventType, code:
this.jsX11KeysymMap[code], modifiers: modifiers});
- } else if(eventType == AjaxViewer.KEY_PRESS && guestos ==
'null') {
+ } else if(eventType == AjaxViewer.KEY_PRESS &&
guestos.toLowerCase() != 'windows') {
--- End diff --
This might be existing code, but for more cleaner way, do we want to remote
all os strings and make them as constants or sort of enums defined at one place
and use them with reference here?
something like:
gustos.toLowerCase() != OsTypes.WINDOWS
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36704745
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands
cmds, VirtualMachineProfile prof
CheckSshCommand check = new
CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+try {
+File uiFiles = new File("systemvm/js");
+if (!uiFiles.exists()) {
+uiFiles = new
File("/usr/share/cloudstack-common/systemvm/js");
+}
+if (uiFiles.exists() && uiFiles.isDirectory()) {
+CopyFileInVmCommand copyFile = new
CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js",
controlNic.getIp4Address());
+cmds.addCommand("copyFile", copyFile);
+} else {
+s_logger.error("Couldn't locate localization files for
console proxy");
+return false;
+}
+} catch (IOException e) {
+s_logger.error("Failed to copy localization files for console
proxy: " + e.getMessage());
--- End diff --
My note is related to catch block, once an io exception is caught, do we
want to return false to callee ( inside the catch block)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36612869
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
--- End diff --
Here if it is a file then I am copying it directly as can be seen in else
block but if it is a directory then I am listing files and then copying files
individually as the library which we are using elsewhere in CloudStack doesn't
support copying directories.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36612463
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
+s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM "
+ cmd.getVmIp(), e);
+return new CopyFileInVmAnswer(cmd, e);
+}
+return new CopyFileInVmAnswer(cmd);
--- End diff --
I got your point but I don't understand how will that make any difference
here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36612331
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands
cmds, VirtualMachineProfile prof
CheckSshCommand check = new
CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+try {
+File uiFiles = new File("systemvm/js");
+if (!uiFiles.exists()) {
+uiFiles = new
File("/usr/share/cloudstack-common/systemvm/js");
+}
+if (uiFiles.exists() && uiFiles.isDirectory()) {
+CopyFileInVmCommand copyFile = new
CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js",
controlNic.getIp4Address());
+cmds.addCommand("copyFile", copyFile);
+} else {
+s_logger.error("Couldn't locate localization files for
console proxy");
+return false;
+}
+} catch (IOException e) {
+s_logger.error("Failed to copy localization files for console
proxy: " + e.getMessage());
--- End diff --
We are copying some files (using pom.xml) in that folder so these files are
expected there. If these files do not exist there then he may have forgotten to
add there those file. Returning false will make him identify the issue because
of that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611905
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands
cmds, VirtualMachineProfile prof
CheckSshCommand check = new
CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+try {
+File uiFiles = new File("systemvm/js");
+if (!uiFiles.exists()) {
+uiFiles = new
File("/usr/share/cloudstack-common/systemvm/js");
+}
+if (uiFiles.exists() && uiFiles.isDirectory()) {
+CopyFileInVmCommand copyFile = new
CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js",
controlNic.getIp4Address());
+cmds.addCommand("copyFile", copyFile);
+} else {
+s_logger.error("Couldn't locate localization files for
console proxy");
+return false;
+}
+} catch (IOException e) {
+s_logger.error("Failed to copy localization files for console
proxy: " + e.getMessage());
+}
+
return true;
}
@Override
public boolean finalizeStart(VirtualMachineProfile profile, long
hostId, Commands cmds, ReservationContext context) {
-CheckSshAnswer answer = (CheckSshAnswer)cmds.getAnswer("checkSsh");
-if (answer == null || !answer.getResult()) {
-if (answer != null) {
-s_logger.warn("Unable to ssh to the VM: " +
answer.getDetails());
-} else {
-s_logger.warn("Unable to ssh to the VM: null answer");
+for(Answer answer : cmds.getAnswers()) {
+if(answer == null || !answer.getResult()) {
+if (answer != null) {
+s_logger.warn("Unable to ssh to the VM: " +
answer.getDetails());
+} else {
+s_logger.warn("Unable to ssh to the VM: null answer");
+}
+if(cmds.getAnswer("copyFile") == answer) {
+continue;
+}
+return false;
--- End diff --
Its returning false if the command is failed or has not returned answer and
that is failure case. In case of copyFile we are ignoring the answer and
continuing. So if block is making sense for that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611661
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
--- End diff --
I mean we are checking whether its directory or not, but if its not a
directory, other types can come EX: block, socket, or other file types defined
by attributes ( like d,b etc), i mean do we have a specific requirement to copy
with specific file type. EX: if we have group of files ( directory, block type,
socket type , plain text files etc) , with above logic it returns all which is
not of directory "type", iam inquiring to see we want to be explicit in code
for a given file type?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611512
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
+s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM "
+ cmd.getVmIp(), e);
+return new CopyFileInVmAnswer(cmd, e);
+}
+return new CopyFileInVmAnswer(cmd);
--- End diff --
I mean in case a copyfilein..if moved inside try block throws an exception
, then the code return new...copyfile inside catch still works.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611296
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
--- End diff --
If its just a file then else part will take care. And I don't understand
how does socket file is involved here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611238
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands
cmds, VirtualMachineProfile prof
CheckSshCommand check = new
CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+try {
+File uiFiles = new File("systemvm/js");
+if (!uiFiles.exists()) {
+uiFiles = new
File("/usr/share/cloudstack-common/systemvm/js");
+}
+if (uiFiles.exists() && uiFiles.isDirectory()) {
+CopyFileInVmCommand copyFile = new
CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js",
controlNic.getIp4Address());
+cmds.addCommand("copyFile", copyFile);
+} else {
+s_logger.error("Couldn't locate localization files for
console proxy");
+return false;
+}
+} catch (IOException e) {
+s_logger.error("Failed to copy localization files for console
proxy: " + e.getMessage());
+}
+
return true;
}
@Override
public boolean finalizeStart(VirtualMachineProfile profile, long
hostId, Commands cmds, ReservationContext context) {
-CheckSshAnswer answer = (CheckSshAnswer)cmds.getAnswer("checkSsh");
-if (answer == null || !answer.getResult()) {
-if (answer != null) {
-s_logger.warn("Unable to ssh to the VM: " +
answer.getDetails());
-} else {
-s_logger.warn("Unable to ssh to the VM: null answer");
+for(Answer answer : cmds.getAnswers()) {
+if(answer == null || !answer.getResult()) {
+if (answer != null) {
+s_logger.warn("Unable to ssh to the VM: " +
answer.getDetails());
+} else {
+s_logger.warn("Unable to ssh to the VM: null answer");
+}
+if(cmds.getAnswer("copyFile") == answer) {
+continue;
+}
+return false;
--- End diff --
It seems this return seems to be inside of if..block, so for loop
effectively has no effect and returns for one run?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611175
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands
cmds, VirtualMachineProfile prof
CheckSshCommand check = new
CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+try {
+File uiFiles = new File("systemvm/js");
+if (!uiFiles.exists()) {
+uiFiles = new
File("/usr/share/cloudstack-common/systemvm/js");
+}
+if (uiFiles.exists() && uiFiles.isDirectory()) {
+CopyFileInVmCommand copyFile = new
CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js",
controlNic.getIp4Address());
+cmds.addCommand("copyFile", copyFile);
+} else {
+s_logger.error("Couldn't locate localization files for
console proxy");
+return false;
+}
+} catch (IOException e) {
+s_logger.error("Failed to copy localization files for console
proxy: " + e.getMessage());
--- End diff --
should we return false in case of an exception?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611140
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
+s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM "
+ cmd.getVmIp(), e);
+return new CopyFileInVmAnswer(cmd, e);
+}
+return new CopyFileInVmAnswer(cmd);
--- End diff --
What purpose will it serve?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611046
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -1279,6 +1285,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+VmwareManager mgr =
getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+File keyFile = mgr.getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
--- End diff --
same as above answer
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611014
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -1279,6 +1285,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+VmwareManager mgr =
getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+File keyFile = mgr.getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
--- End diff --
No. There is no file open operation involved here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36609308
--- Diff:
plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java
---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+File keyFile = getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile,
null, cmd.getDest(), file.getCanonicalPath(), null);
+}
+}
+} catch (Exception e) {
+s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM "
+ cmd.getVmIp(), e);
+return new CopyFileInVmAnswer(cmd, e);
+}
+return new CopyFileInVmAnswer(cmd);
--- End diff --
Should this be moved inside to end of try block?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36609278
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
--- End diff --
@sedukull Could you please elaborate more on what is repetitive here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36609212
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+SshHelper.scpTo(cmd.getVmIp(), 3922, "root",
keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+}
+} else {
--- End diff --
If its not a directory, but can be a block or socket file possible? Is
there any criteria check to be added for file type to be copied?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36609071
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -1279,6 +1285,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+VmwareManager mgr =
getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+File keyFile = mgr.getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
+if(file.exists()) {
+if(file.isDirectory()) {
+for (File f : FileUtils.listFiles(new
File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
--- End diff --
As well, here if its a directory, we are using multiple file objects, are
these auto closed and resources consumed are released back or does the api for
File..provides that facility?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36609019
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -1279,6 +1285,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+VmwareManager mgr =
getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+File keyFile = mgr.getSystemVMKeyFile();
+try {
+File file = new File(cmd.getSrc());
--- End diff --
Does file object consumes a resource to be flushed or closed post the usage
or its auto closed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36608900
--- Diff:
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java
---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends
CommandWrapper {
+
+private static final Logger s_logger =
Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+@Override public Answer execute(CopyFileInVmCommand cmd,
LibvirtComputingResource libvirtComputingResource) {
+final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+try {
+File file = new File(cmd.getSrc());
--- End diff --
This code seems repetitive, can this be made as a lib, or convert to a
factory based upon given hypervisor type and use that pattern in each command
execute call?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
GitHub user anshul1886 opened a pull request: https://github.com/apache/cloudstack/pull/669 Made the adding new keyboard language support easier This branch has implemented following improvements in console proxy keyboard language support 1) ajaxviewer.js and ajaxkeys.js are main files involved in key code translations. These files now can be copied in systemvm/js folder and they will be copied to CPVM with stop/start performed on it. 2) Started passing parameters to CPVM needed to resolve the ambiguous cases of keycode translations. 3) Generalise the framework such that one needs to modify only ajaxkeys.js (file which has keycode mappings) without need of much knowledge in js. You can merge this pull request into a Git repository by running: $ git pull https://github.com/anshul1886/cloudstack-1 nonuskeyboardsupport Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/669.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #669 commit 94995312a97716417cd0bbe7c2a323aa5cc09f5e Author: Anshul Gangwar Date: 2015-08-04T04:54:16Z Added support for copying js files from management server to console proxy VM with stop start commit 9e5f1c7b071f48d3c608b314e8111f77a0018a0d Author: Anshul Gangwar Date: 2015-08-07T05:45:03Z Added CopyFileInVmCommand support in ovm3 and simulator and added fallback path for js files commit 130827f928cae4f56cd76a9431745f5959d50349 Author: Anshul Gangwar Date: 2015-08-07T08:38:32Z generalize console proxy keyboard support framework and added the additional parameters which will be needed for keyboard mappings commit 2d20c2c9b0bfa11157f3412080144df81d149438 Author: Anshul Gangwar Date: 2015-08-10T06:27:03Z Moved the console Keyboard Options to new file so that user can add keyboard options easily --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
