[sip-comm-dev] Re: suggestions


#1

Hi Damian,

Thanks a lot for sending me bugs :slight_smile:
I have attached a patch fixing them.

Am using
"diff -x CVS -x nbproject -u -r -N sip-communicator-cvs sip-communicator >
ssh4sc.patch"
to generate the patch, is their a better way?
(both sip-communicator-cvs and sip-communicator is updated from CVS first,
am a little unsure on cvs diff)

rest inline

Hi again :slight_smile:

there is one more thing I forgot, but I don't know how to solve it.
if you try a command like ping on linux machine it doesn't stop until
killed. and so you keep receive
messages and cannot do anything. Have you got some idea on this ?

damencho

yep this is a known problem, earlier i thought of developing GUI plugins for
Ctrl+Z/C/D etc
but as the GUI plugins are not protocol specific the idea was dropped.

am thinking of it implementing like commands of ^C, ^D etc
(^ specifies its a special command)

Damian Minkov wrote:

> Hi,
>
> I have few questions/suggestions :
>
> 1. When creating a contact and haven't supplied a username I think you
> can use the username of the current user :
> System.property - user.name

fixed

2. When you are adding a contact and the details form pops and you
> choose add detail : the below fields become editble.
> but you cannot back your action : remove the check box add details.

yes, i did it intentionally, (seems better to me :slight_smile: ), please comment

3. On my machine I have alias in /etc/hosts for my office machine :
> office some_ip. So I've created a new contact and filled only office
> as host. the contact is created abd here whats in the contact.xml
>

fixed

thanks

ssh4sc.tar.bz2 (205 KB)

路路路

On 8/17/07, Damian Minkov <damencho@damencho.com> wrote:
--
Shobhit Jindal


#2

Hi all, Jindal,

I finally reviewed your ssh patch (which was really huge :)).
First I'd like to say that you've done a very good job, your code is clear, understandable and well structured and the amount of work is impressive.

However, I found a security problem during my review so I decided to not activate your code for the moment.
Here are the little things I've noticed on your code and which need a clarification or a correction :slight_smile: (don't worry about the amount of question, it's quite normal and it absolutely doesn't mean that your code has problems everywhere)

* there were some javadoc / 80 characters deadline problems and some are still not fixed but as cruiseControl will probably cry, I'll probably correct them these days

in ContactSSHFileTransferDaemon
* I don't really understand the name of this class. Why contact ?
* constructor :

public ContactSSHFileTransferDaemon(
聽聽聽聽聽聽聽聽聽聽聽聽ContactSSH sshContact,
聽聽聽聽聽聽聽聽聽聽聽聽OperationSetPersistentPresenceSSHImpl opSetPersPresence,
聽聽聽聽聽聽聽聽聽聽聽聽OperationSetBasicInstantMessagingSSHImpl instantMessaging,
聽聽聽聽聽聽聽聽聽聽聽聽ProtocolProviderServiceSSHImpl ppService)

I think that all the operationSets are deductibles from the ppService, aren't they ?
聽聽聽聽聽聽聽聽聽聽聽* upload / dowload : is it possible to have many requests for an upload or a download in the same time ? If the answer is yes, your code will probably fail as you're using global variables to store the parameters of the operations

* you're display exception in the chat window, I think that you're doing the logger's job, you should probably just need to use the logger for this

* line 408 : for(int i=0;;i++) <- very surprising construction, I didn't even know that a such construction was permitted :slight_smile:

* line 437 you're treating an error case but you don't throw an exception here. Why ?

ContactSSH
I think that this specialized interface is a really good idea !

ContactSSHReaderDaemon
* Why "contact" in this class name ? (again)

* You're waiting 50ms for a message to come but, to me, it seems very CPU consuming. 500ms isn't sufficient ?

* !!! big fat warning !!! (here is the security problem)

When you're reading the messages, you're putting in a buffer everything received until having 250ms during which nothing is transmitted.
Now think a little bit on this :
Jay is really happy, SC now support SSH, he is very impatient and wants to test it NOW ! so he started his local server and, using his brand new Gigabyte network, he connects himself to his server. Now, to test if everything is ok, he types "cat /proc/random" and nothing happens but a few seconds later, his java VM crashes.
It's simply because, you added the server answer in the buffer again and again until reaching the end of the memory.

A simple fix for it is to set a limit to the buffer, when the limit is reached, simply display the content of the buffer until the last '\n' to keep a coherent display.

This MUST be corrected before activating SSH in SC.

MessageSSHImpl
* you consider null as an equivalent of plain/text - UTF8. Isn't it a little bit dangerous for the future developments ? (risks of NullPointerException)

OpSetBasicIM
* like before, the opset can be deducted from the ppService, so aren't needed as parameters

* isContentTypeSupported never returns true. Does it means that you're not able to send any sort of content ?

OpSetContactInfo
* There are no type verification especially for the port number which remains a String. Isn't it a little bit dangerous for other classes which will perhaps try to use it as an Integer ?

* Why OperationSet in the class name ?

OpSetContactTimer

* Why OperationSet in the class name ?

OpSetFileTransfert

* like before, some parameters in the constructor aren't needed

* assert false at the end ?

* you use file.separator in this class and it's a very good idea but just after you use a dirty '/', why ?

OpSetPersistentPresence

* subscribe : sshContact.getSSHConfigurationForm().setVisible(true); What's that ?

* createUnresolvedContact : startTimerTask is perhaps not needed as, normally, an unresolved contact doesn't generate network communications.

* use protocolNames for the name of the service in the Osgi requests

* why unresolvedContact have resolved=true and volatile resolved=false ?

ppFactorySSHImpl

* use ProtocolNames.SSH here again

ppFactorySSH
* again, this class is a clever solution !

ppServiceSSHImpl
* use ProtocolNames.SSH here again

Resources (both)

* getImage : the array has a fixed size, it's dirty and may cause some problem, why not use the InputStream.available to fix a dynamic size for the byte array ?

resources.properties
* Some parameters haven't their place here like the timeout value. To see where to put these parameters, take a look at what Yana do to save the last status of an account.

* the testCommand and response aren't used, why keep it on this file ?

SSHActivator

* use ProtocolNames.SSH here again

SSHUserInfo

* promptYesNo : The dialog type is "warning" however you're asking a question so why didn't you chose the "question" type ?

resources.properties in the wizzard

* use ProtocolNames.SSH here again for the protocol name

SSHAccountRegistration

* identityFile : I dont understand what it is. Can you tell it in the javadoc ?

* Why the server port is a String and not an int in the methods ?

And that's all.
Once again, don't worry about this list, you've done a great job Jindal !
However if you want your baby live in SC, you'll need to provide us a little patch which correct all this and especially the little security problem.

Cheers,
Ben

路路路

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#3

Hi Benoit,

Sorry for the really long delay!

Thanks for reviewing my patch :slight_smile: .. I am attaching SSH protocol impl and
the wizard files with this mail which incorporate many changes as described
below..
(i tried to create a patch with svn diff but it didnt incorporate newly
created files and delete non existing ones)

Hi all, Jindal,

I finally reviewed your ssh patch (which was really huge :)).
First I'd like to say that you've done a very good job, your code is
clear, understandable and well structured and the amount of work is
impressive.

However, I found a security problem during my review so I decided to not
activate your code for the moment.
Here are the little things I've noticed on your code and which need a
clarification or a correction :slight_smile: (don't worry about the amount of
question, it's quite normal and it absolutely doesn't mean that your
code has problems everywhere)

* there were some javadoc / 80 characters deadline problems and some are
still not fixed but as cruiseControl will probably cry, I'll probably
correct them these days

I didnt get this :slight_smile: please explain

in ContactSSHFileTransferDaemon

* I don't really understand the name of this class. Why contact ?
* constructor :

Corrected
In the start i tried to maintain a clear difference between classes specific
to ssh account and a contact.. its no more needed

public ContactSSHFileTransferDaemon(

聽聽聽聽聽聽聽聽聽聽聽聽ContactSSH sshContact,
聽聽聽聽聽聽聽聽聽聽聽聽OperationSetPersistentPresenceSSHImpl opSetPersPresence,
聽聽聽聽聽聽聽聽聽聽聽聽OperationSetBasicInstantMessagingSSHImpl instantMessaging,
聽聽聽聽聽聽聽聽聽聽聽聽ProtocolProviderServiceSSHImpl ppService)

I think that all the operationSets are deductibles from the ppService,
aren't they ?

Corrected

* upload / dowload : is it possible to have many requests for an upload

or a download in the same time ? If the answer is yes, your code will
probably fail as you're using global variables to store the parameters
of the operations

which global variables specifically?
A new instance of this daemon is created for each upload/download ..
simulataneous upload/downloads seem to be working fine

* you're display exception in the chat window, I think that you're doing

the logger's job, you should probably just need to use the logger for this

In case of any error I need to notify the user somehow ... giving a generic
error may not be of much use .. so am displaying the exception to the user
(its also logged as an error)

* line 408 : for(int i=0;;i++) <- very surprising construction,

I didn't even know that a such construction was permitted :slight_smile:

lol :slight_smile: any better way to increment a variable for an infinite loop?

* line 437 you're treating an error case but you don't throw an

exception here. Why ?

its a break condition .. (IO errors are automatically thrown by the method
in throws clause)

ContactSSH

I think that this specialized interface is a really good idea !

ContactSSHReaderDaemon
* Why "contact" in this class name ? (again)

Corrected

* You're waiting 50ms for a message to come but, to me, it seems very

CPU consuming. 500ms isn't sufficient ?

Reader daemon checks for any new messages every 50ms and then buffers till
250ms if there is any.. and am sleeping instead of spinlocking ..

* !!! big fat warning !!! (here is the security problem)

When you're reading the messages, you're putting in a buffer everything
received until having 250ms during which nothing is transmitted.
Now think a little bit on this :
Jay is really happy, SC now support SSH, he is very impatient and wants
to test it NOW ! so he started his local server and, using his brand new
Gigabyte network, he connects himself to his server. Now, to test if
everything is ok, he types "cat /proc/random" and nothing happens but a
few seconds later, his java VM crashes.
It's simply because, you added the server answer in the buffer again and
again until reaching the end of the memory.

Agreed :slight_smile:

A simple fix for it is to set a limit to the buffer, when the limit is

reached, simply display the content of the buffer until the last '\n' to
keep a coherent display.

This MUST be corrected before activating SSH in SC.

Corrected .. a buffer limit of 32700 has been set.

MessageSSHImpl

* you consider null as an equivalent of plain/text - UTF8. Isn't it a
little bit dangerous for the future developments ? (risks of
NullPointerException)

presently am not using/checking for content type of message anywhere .. so
its set to null

OpSetBasicIM

* like before, the opset can be deducted from the ppService, so aren't
needed as parameters

Corrected

* isContentTypeSupported never returns true. Does it means that you're

not able to send any sort of content ?

method is never called :slight_smile: as content type is of no relevance presently

OpSetContactInfo

* There are no type verification especially for the port number which
remains a String. Isn't it a little bit dangerous for other classes
which will perhaps try to use it as an Integer ?

Corrected by making port text field accept only digits (max 5)
Am trying to maintain contact info at one place - in the GUI form where port
represented by a textfield ..

* Why OperationSet in the class name ?

Corrected

OpSetContactTimer

* Why OperationSet in the class name ?

Corrected

OpSetFileTransfert

* like before, some parameters in the constructor aren't needed

Corrected

* assert false at the end ?

Presently am unsure of how will interfaces of generic file transfer for SC
will evolve ..
Hence am going for non-standard assumptions and making an implementation
specific for SSH File transfer...
there may be more than 2 modes of transfer in generic file transfer but SSH
one supports only 2 .. so asserting false at end may help...

* you use file.separator in this class and it's a very good idea but

just after you use a dirty '/', why ?

Corrected :slight_smile:

OpSetPersistentPresence

* subscribe : sshContact.getSSHConfigurationForm().setVisible(true);
What's that ?

As soon as a contact is created[subscribed] .. the user must provide server
name/ip address of the remote machine before its added to contacts list ...

The above makes the config form of remote machine visible in the subsribe
method..

* createUnresolvedContact : startTimerTask is perhaps not needed as,

normally, an unresolved contact doesn't generate network communications.

Contacts can only be resolved the Timer Task [update its status in Contact
list]
anyways timer task just checks for reachability [just an innocent open/close
on ssh port presently]

* use protocolNames for the name of the service in the Osgi requests

I didnt get the above .. same for all protocolNames.SSH below

* why unresolvedContact have resolved=true and volatile resolved=false ?

there are no volatile contacts in SSH .. this code is redundant
[an unknown/unspecified machine can't send a messages to a user]

ppFactorySSHImpl

* use ProtocolNames.SSH here again

didn't get this

ppFactorySSH

* again, this class is a clever solution !

ppServiceSSHImpl
* use ProtocolNames.SSH here again

didn't get this

Resources (both)

* getImage : the array has a fixed size, it's dirty and may cause some
problem, why not use the InputStream.available to fix a dynamic size for
the byte array ?

Corrected

resources.properties

* Some parameters haven't their place here like the timeout value. To
see where to put these parameters, take a look at what Yana do to save
the last status of an account.

* the testCommand and response aren't used, why keep it on this file ?

These are temporary solutions to tricky problems .. a better soln needs to
be found.

SSHActivator

* use ProtocolNames.SSH here again

didn't get this

SSHUserInfo

* promptYesNo : The dialog type is "warning" however you're asking a
question so why didn't you chose the "question" type ?

Corrected

resources.properties in the wizzard

* use ProtocolNames.SSH here again for the protocol name

didn't get this

SSHAccountRegistration

* identityFile : I dont understand what it is. Can you tell it in the
javadoc ?

Identity file is a private[default] key of the user which is one of the
methods of authentication [its a standard in SSH]

* Why the server port is a String and not an int in the methods ?

same as in OpSetContactInfo

And that's all.

ssh.zip (79.8 KB)

路路路

On 9/28/07, Benoit Pradelle <ze_real_neo@yahoo.fr> wrote:

Once again, don't worry about this list, you've done a great job Jindal !
However if you want your baby live in SC, you'll need to provide us a
little patch which correct all this and especially the little security
problem.

Cheers,
Ben

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

--
Shobhit Jindal


#4

Does this SSH feature in S-C work with the encryption in Asterisk
v1.4.x? If not, which SIP server does it work with? Or is it only for
P2P S-C sessions?

路路路

On Sat, 2007-10-27 at 01:09 +0530, Shobhit Jindal wrote:

Hi Benoit,

Sorry for the really long delay!

Thanks for reviewing my patch :slight_smile: .. I am attaching SSH protocol impl
and the wizard files with this mail which incorporate many changes as
described below..
(i tried to create a patch with svn diff but it didnt incorporate
newly created files and delete non existing ones)

On 9/28/07, Benoit Pradelle <ze_real_neo@yahoo.fr> wrote:
聽聽聽聽聽聽聽聽Hi all, Jindal,
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽I finally reviewed your ssh patch (which was really huge :)).
聽聽聽聽聽聽聽聽First I'd like to say that you've done a very good job, your
聽聽聽聽聽聽聽聽code is
聽聽聽聽聽聽聽聽clear, understandable and well structured and the amount of
聽聽聽聽聽聽聽聽work is
聽聽聽聽聽聽聽聽impressive.
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽However, I found a security problem during my review so I
聽聽聽聽聽聽聽聽decided to not
聽聽聽聽聽聽聽聽activate your code for the moment.
聽聽聽聽聽聽聽聽Here are the little things I've noticed on your code and which
聽聽聽聽聽聽聽聽need a
聽聽聽聽聽聽聽聽clarification or a correction :slight_smile: (don't worry about the amount
聽聽聽聽聽聽聽聽of
聽聽聽聽聽聽聽聽question, it's quite normal and it absolutely doesn't mean
聽聽聽聽聽聽聽聽that your
聽聽聽聽聽聽聽聽code has problems everywhere)
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* there were some javadoc / 80 characters deadline problems
聽聽聽聽聽聽聽聽and some are
聽聽聽聽聽聽聽聽still not fixed but as cruiseControl will probably cry, I'll
聽聽聽聽聽聽聽聽probably
聽聽聽聽聽聽聽聽correct them these days

I didnt get this :slight_smile: please explain

聽聽聽聽聽聽聽聽in ContactSSHFileTransferDaemon
聽聽聽聽聽聽聽聽* I don't really understand the name of this class. Why
聽聽聽聽聽聽聽聽contact ?
聽聽聽聽聽聽聽聽* constructor :

Corrected
In the start i tried to maintain a clear difference between classes
specific to ssh account and a contact.. its no more needed

聽聽聽聽聽聽聽聽public ContactSSHFileTransferDaemon(
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽ContactSSH sshContact,
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽OperationSetPersistentPresenceSSHImpl
聽聽聽聽聽聽聽聽opSetPersPresence,
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽OperationSetBasicInstantMessagingSSHImpl
聽聽聽聽聽聽聽聽instantMessaging,
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽ProtocolProviderServiceSSHImpl ppService)
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽I think that all the operationSets are deductibles from the
聽聽聽聽聽聽聽聽ppService,
聽聽聽聽聽聽聽聽aren't they ?

Corrected

聽聽聽聽聽聽聽聽* upload / dowload : is it possible to have many requests for
聽聽聽聽聽聽聽聽an upload
聽聽聽聽聽聽聽聽or a download in the same time ? If the answer is yes, your
聽聽聽聽聽聽聽聽code will
聽聽聽聽聽聽聽聽probably fail as you're using global variables to store the
聽聽聽聽聽聽聽聽parameters
聽聽聽聽聽聽聽聽of the operations

which global variables specifically?
A new instance of this daemon is created for each upload/download ..

simulataneous upload/downloads seem to be working fine

聽聽聽聽聽聽聽聽* you're display exception in the chat window, I think that
聽聽聽聽聽聽聽聽you're doing
聽聽聽聽聽聽聽聽the logger's job, you should probably just need to use the
聽聽聽聽聽聽聽聽logger for this

In case of any error I need to notify the user somehow ... giving a
generic error may not be of much use .. so am displaying the
exception to the user (its also logged as an error)

聽聽聽聽聽聽聽聽* line 408 : for(int i=0;;i++) <- very surprising
聽聽聽聽聽聽聽聽construction,
聽聽聽聽聽聽聽聽I didn't even know that a such construction was permitted :slight_smile:

lol :slight_smile: any better way to increment a variable for an infinite loop?

聽聽聽聽聽聽聽聽* line 437 you're treating an error case but you don't throw
聽聽聽聽聽聽聽聽an
聽聽聽聽聽聽聽聽exception here. Why ?
its a break condition .. (IO errors are automatically thrown by the
method in throws clause)

聽聽聽聽聽聽聽聽ContactSSH
聽聽聽聽聽聽聽聽I think that this specialized interface is a really good
聽聽聽聽聽聽聽聽idea !
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽ContactSSHReaderDaemon
聽聽聽聽聽聽聽聽* Why "contact" in this class name ? (again)

Corrected

聽聽聽聽聽聽聽聽* You're waiting 50ms for a message to come but, to me, it
聽聽聽聽聽聽聽聽seems very
聽聽聽聽聽聽聽聽CPU consuming. 500ms isn't sufficient ?

Reader daemon checks for any new messages every 50ms and then buffers
till 250ms if there is any.. and am sleeping instead of
spinlocking ..

聽聽聽聽聽聽聽聽* !!! big fat warning !!! (here is the security problem)
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽When you're reading the messages, you're putting in a buffer
聽聽聽聽聽聽聽聽everything
聽聽聽聽聽聽聽聽received until having 250ms during which nothing is
聽聽聽聽聽聽聽聽transmitted.
聽聽聽聽聽聽聽聽Now think a little bit on this :
聽聽聽聽聽聽聽聽Jay is really happy, SC now support SSH, he is very impatient
聽聽聽聽聽聽聽聽and wants
聽聽聽聽聽聽聽聽to test it NOW ! so he started his local server and, using his
聽聽聽聽聽聽聽聽brand new
聽聽聽聽聽聽聽聽Gigabyte network, he connects himself to his server. Now, to
聽聽聽聽聽聽聽聽test if
聽聽聽聽聽聽聽聽everything is ok, he types "cat /proc/random" and nothing
聽聽聽聽聽聽聽聽happens but a
聽聽聽聽聽聽聽聽few seconds later, his java VM crashes.
聽聽聽聽聽聽聽聽It's simply because, you added the server answer in the buffer
聽聽聽聽聽聽聽聽again and
聽聽聽聽聽聽聽聽again until reaching the end of the memory.

Agreed :slight_smile:

聽聽聽聽聽聽聽聽A simple fix for it is to set a limit to the buffer, when the
聽聽聽聽聽聽聽聽limit is
聽聽聽聽聽聽聽聽reached, simply display the content of the buffer until the
聽聽聽聽聽聽聽聽last '\n' to
聽聽聽聽聽聽聽聽keep a coherent display.
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽This MUST be corrected before activating SSH in SC.

Corrected .. a buffer limit of 32700 has been set.

聽聽聽聽聽聽聽聽MessageSSHImpl
聽聽聽聽聽聽聽聽* you consider null as an equivalent of plain/text - UTF8.
聽聽聽聽聽聽聽聽Isn't it a
聽聽聽聽聽聽聽聽little bit dangerous for the future developments ? (risks of
聽聽聽聽聽聽聽聽NullPointerException)

presently am not using/checking for content type of message
anywhere .. so its set to null

聽聽聽聽聽聽聽聽OpSetBasicIM
聽聽聽聽聽聽聽聽* like before, the opset can be deducted from the ppService,
聽聽聽聽聽聽聽聽so aren't
聽聽聽聽聽聽聽聽needed as parameters

Corrected

聽聽聽聽聽聽聽聽* isContentTypeSupported never returns true. Does it means
聽聽聽聽聽聽聽聽that you're
聽聽聽聽聽聽聽聽not able to send any sort of content ?

method is never called :slight_smile: as content type is of no relevance
presently

聽聽聽聽聽聽聽聽OpSetContactInfo
聽聽聽聽聽聽聽聽* There are no type verification especially for the port
聽聽聽聽聽聽聽聽number which
聽聽聽聽聽聽聽聽remains a String. Isn't it a little bit dangerous for other
聽聽聽聽聽聽聽聽classes
聽聽聽聽聽聽聽聽which will perhaps try to use it as an Integer ?

Corrected by making port text field accept only digits (max 5)
Am trying to maintain contact info at one place - in the GUI form
where port represented by a textfield ..

聽聽聽聽聽聽聽聽* Why OperationSet in the class name ?

Corrected

聽聽聽聽聽聽聽聽OpSetContactTimer
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* Why OperationSet in the class name ?

Corrected

聽聽聽聽聽聽聽聽OpSetFileTransfert
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* like before, some parameters in the constructor aren't
聽聽聽聽聽聽聽聽needed

Corrected

聽聽聽聽聽聽聽聽* assert false at the end ?

Presently am unsure of how will interfaces of generic file transfer
for SC will evolve ..

Hence am going for non-standard assumptions and making an
implementation specific for SSH File transfer...
there may be more than 2 modes of transfer in generic file transfer
but SSH one supports only 2 .. so asserting false at end may help...

聽聽聽聽聽聽聽聽* you use file.separator in this class and it's a very good
聽聽聽聽聽聽聽聽idea but
聽聽聽聽聽聽聽聽just after you use a dirty '/', why ?

Corrected :slight_smile:

聽聽聽聽聽聽聽聽OpSetPersistentPresence
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* subscribe :
聽聽聽聽聽聽聽聽sshContact.getSSHConfigurationForm().setVisible(true);
聽聽聽聽聽聽聽聽What's that ?

As soon as a contact is created[subscribed] .. the user must provide
server name/ip address of the remote machine before its added to
contacts list ...

The above makes the config form of remote machine visible in the
subsribe method..

聽聽聽聽聽聽聽聽* createUnresolvedContact : startTimerTask is perhaps not
聽聽聽聽聽聽聽聽needed as,
聽聽聽聽聽聽聽聽normally, an unresolved contact doesn't generate network
聽聽聽聽聽聽聽聽communications.

Contacts can only be resolved the Timer Task [update its status in
Contact list]
anyways timer task just checks for reachability [just an innocent
open/close on ssh port presently]

聽聽聽聽聽聽聽聽* use protocolNames for the name of the service in the Osgi
聽聽聽聽聽聽聽聽requests

I didnt get the above .. same for all protocolNames.SSH below

聽聽聽聽聽聽聽聽* why unresolvedContact have resolved=true and volatile
聽聽聽聽聽聽聽聽resolved=false ?

there are no volatile contacts in SSH .. this code is redundant
[an unknown/unspecified machine can't send a messages to a user]

聽聽聽聽聽聽聽聽ppFactorySSHImpl
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* use ProtocolNames.SSH here again

didn't get this

聽聽聽聽聽聽聽聽ppFactorySSH
聽聽聽聽聽聽聽聽* again, this class is a clever solution !
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽ppServiceSSHImpl
聽聽聽聽聽聽聽聽* use ProtocolNames.SSH here again

didn't get this

聽聽聽聽聽聽聽聽Resources (both)
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* getImage : the array has a fixed size, it's dirty and may
聽聽聽聽聽聽聽聽cause some
聽聽聽聽聽聽聽聽problem, why not use the InputStream.available to fix a
聽聽聽聽聽聽聽聽dynamic size for
聽聽聽聽聽聽聽聽the byte array ?

Corrected

聽聽聽聽聽聽聽聽resources.properties
聽聽聽聽聽聽聽聽* Some parameters haven't their place here like the timeout
聽聽聽聽聽聽聽聽value. To
聽聽聽聽聽聽聽聽see where to put these parameters, take a look at what Yana do
聽聽聽聽聽聽聽聽to save
聽聽聽聽聽聽聽聽the last status of an account.
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* the testCommand and response aren't used, why keep it on
聽聽聽聽聽聽聽聽this file ?

These are temporary solutions to tricky problems .. a better soln
needs to be found.

聽聽聽聽聽聽聽聽SSHActivator
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* use ProtocolNames.SSH here again

didn't get this

聽聽聽聽聽聽聽聽SSHUserInfo
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* promptYesNo : The dialog type is "warning" however you're
聽聽聽聽聽聽聽聽asking a
聽聽聽聽聽聽聽聽question so why didn't you chose the "question" type ?

Corrected

聽聽聽聽聽聽聽聽resources.properties in the wizzard
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* use ProtocolNames.SSH here again for the protocol name

didn't get this

聽聽聽聽聽聽聽聽SSHAccountRegistration
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* identityFile : I dont understand what it is. Can you tell it
聽聽聽聽聽聽聽聽in the
聽聽聽聽聽聽聽聽javadoc ?

Identity file is a private[default] key of the user which is one of
the methods of authentication [its a standard in SSH]

聽聽聽聽聽聽聽聽* Why the server port is a String and not an int in the
聽聽聽聽聽聽聽聽methods ?

same as in OpSetContactInfo

聽聽聽聽聽聽聽聽And that's all.
聽聽聽聽聽聽聽聽Once again, don't worry about this list, you've done a great
聽聽聽聽聽聽聽聽job Jindal !
聽聽聽聽聽聽聽聽However if you want your baby live in SC, you'll need to
聽聽聽聽聽聽聽聽provide us a
聽聽聽聽聽聽聽聽little patch which correct all this and especially the little
聽聽聽聽聽聽聽聽security
聽聽聽聽聽聽聽聽problem.
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽Cheers,
聽聽聽聽聽聽聽聽Ben
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽---------------------------------------------------------------------
聽聽聽聽聽聽聽聽To unsubscribe, e-mail:
聽聽聽聽聽聽聽聽dev-unsubscribe@sip-communicator.dev.java.net
聽聽聽聽聽聽聽聽For additional commands, e-mail:
聽聽聽聽聽聽聽聽dev-help@sip-communicator.dev.java.net
聽聽聽聽聽聽聽聽
--
Shobhit Jindal
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

--

(C) Matthew Rubenstein

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#5

SSH feature is for P2p S-C sessions only.

路路路

-
Shobhit

On 10/27/07, Matthew Rubenstein <email@mattruby.com> wrote:

聽聽聽聽聽聽聽聽Does this SSH feature in S-C work with the encryption in Asterisk
v1.4.x? If not, which SIP server does it work with? Or is it only for
P2P S-C sessions?

On Sat, 2007-10-27 at 01:09 +0530, Shobhit Jindal wrote:
> Hi Benoit,
>
> Sorry for the really long delay!
>
> Thanks for reviewing my patch :slight_smile: .. I am attaching SSH protocol impl
> and the wizard files with this mail which incorporate many changes as
> described below..
> (i tried to create a patch with svn diff but it didnt incorporate
> newly created files and delete non existing ones)
>
> On 9/28/07, Benoit Pradelle <ze_real_neo@yahoo.fr> wrote:
> Hi all, Jindal,
>
> I finally reviewed your ssh patch (which was really huge :)).
> First I'd like to say that you've done a very good job, your
> code is
> clear, understandable and well structured and the amount of
> work is
> impressive.
>
> However, I found a security problem during my review so I
> decided to not
> activate your code for the moment.
> Here are the little things I've noticed on your code and which
> need a
> clarification or a correction :slight_smile: (don't worry about the amount
> of
> question, it's quite normal and it absolutely doesn't mean
> that your
> code has problems everywhere)
>
> * there were some javadoc / 80 characters deadline problems
> and some are
> still not fixed but as cruiseControl will probably cry, I'll
> probably
> correct them these days
>
> I didnt get this :slight_smile: please explain
>
>
> in ContactSSHFileTransferDaemon
> * I don't really understand the name of this class. Why
> contact ?
> * constructor :
>
> Corrected
> In the start i tried to maintain a clear difference between classes
> specific to ssh account and a contact.. its no more needed
>
>
> public ContactSSHFileTransferDaemon(
> ContactSSH sshContact,
> OperationSetPersistentPresenceSSHImpl
> opSetPersPresence,
> OperationSetBasicInstantMessagingSSHImpl
> instantMessaging,
> ProtocolProviderServiceSSHImpl ppService)
>
> I think that all the operationSets are deductibles from the
> ppService,
> aren't they ?
>
> Corrected
>
>
> * upload / dowload : is it possible to have many requests for
> an upload
> or a download in the same time ? If the answer is yes, your
> code will
> probably fail as you're using global variables to store the
> parameters
> of the operations
>
> which global variables specifically?
> A new instance of this daemon is created for each upload/download ..
>
> simulataneous upload/downloads seem to be working fine
>
> * you're display exception in the chat window, I think that
> you're doing
> the logger's job, you should probably just need to use the
> logger for this
>
> In case of any error I need to notify the user somehow ... giving a
> generic error may not be of much use .. so am displaying the
> exception to the user (its also logged as an error)
>
>
> * line 408 : for(int i=0;;i++) <- very surprising
> construction,
> I didn't even know that a such construction was permitted :slight_smile:
>
> lol :slight_smile: any better way to increment a variable for an infinite loop?
>
>
> * line 437 you're treating an error case but you don't throw
> an
> exception here. Why ?
> its a break condition .. (IO errors are automatically thrown by the
> method in throws clause)
>
>
> ContactSSH
> I think that this specialized interface is a really good
> idea !
>
>
> ContactSSHReaderDaemon
> * Why "contact" in this class name ? (again)
>
> Corrected
>
>
> * You're waiting 50ms for a message to come but, to me, it
> seems very
> CPU consuming. 500ms isn't sufficient ?
>
> Reader daemon checks for any new messages every 50ms and then buffers
> till 250ms if there is any.. and am sleeping instead of
> spinlocking ..
>
> * !!! big fat warning !!! (here is the security problem)
>
> When you're reading the messages, you're putting in a buffer
> everything
> received until having 250ms during which nothing is
> transmitted.
> Now think a little bit on this :
> Jay is really happy, SC now support SSH, he is very impatient
> and wants
> to test it NOW ! so he started his local server and, using his
> brand new
> Gigabyte network, he connects himself to his server. Now, to
> test if
> everything is ok, he types "cat /proc/random" and nothing
> happens but a
> few seconds later, his java VM crashes.
> It's simply because, you added the server answer in the buffer
> again and
> again until reaching the end of the memory.
>
> Agreed :slight_smile:
>
>
> A simple fix for it is to set a limit to the buffer, when the
> limit is
> reached, simply display the content of the buffer until the
> last '\n' to
> keep a coherent display.
>
> This MUST be corrected before activating SSH in SC.
>
> Corrected .. a buffer limit of 32700 has been set.
>
>
> MessageSSHImpl
> * you consider null as an equivalent of plain/text - UTF8.
> Isn't it a
> little bit dangerous for the future developments ? (risks of
> NullPointerException)
>
> presently am not using/checking for content type of message
> anywhere .. so its set to null
>
>
> OpSetBasicIM
> * like before, the opset can be deducted from the ppService,
> so aren't
> needed as parameters
>
> Corrected
>
>
> * isContentTypeSupported never returns true. Does it means
> that you're
> not able to send any sort of content ?
>
> method is never called :slight_smile: as content type is of no relevance
> presently
>
>
> OpSetContactInfo
> * There are no type verification especially for the port
> number which
> remains a String. Isn't it a little bit dangerous for other
> classes
> which will perhaps try to use it as an Integer ?
>
> Corrected by making port text field accept only digits (max 5)
> Am trying to maintain contact info at one place - in the GUI form
> where port represented by a textfield ..
>
>
> * Why OperationSet in the class name ?
>
> Corrected
>
>
> OpSetContactTimer
>
> * Why OperationSet in the class name ?
>
> Corrected
>
>
> OpSetFileTransfert
>
> * like before, some parameters in the constructor aren't
> needed
>
> Corrected
>
>
> * assert false at the end ?
>
> Presently am unsure of how will interfaces of generic file transfer
> for SC will evolve ..
>
> Hence am going for non-standard assumptions and making an
> implementation specific for SSH File transfer...
> there may be more than 2 modes of transfer in generic file transfer
> but SSH one supports only 2 .. so asserting false at end may help...
>
> * you use file.separator in this class and it's a very good
> idea but
> just after you use a dirty '/', why ?
>
> Corrected :slight_smile:
>
>
> OpSetPersistentPresence
>
> * subscribe :
> sshContact.getSSHConfigurationForm().setVisible(true);
> What's that ?
>
> As soon as a contact is created[subscribed] .. the user must provide
> server name/ip address of the remote machine before its added to
> contacts list ...
>
>
> The above makes the config form of remote machine visible in the
> subsribe method..
>
> * createUnresolvedContact : startTimerTask is perhaps not
> needed as,
> normally, an unresolved contact doesn't generate network
> communications.
>
> Contacts can only be resolved the Timer Task [update its status in
> Contact list]
> anyways timer task just checks for reachability [just an innocent
> open/close on ssh port presently]
>
> * use protocolNames for the name of the service in the Osgi
> requests
>
> I didnt get the above .. same for all protocolNames.SSH below
>
>
> * why unresolvedContact have resolved=true and volatile
> resolved=false ?
>
> there are no volatile contacts in SSH .. this code is redundant
> [an unknown/unspecified machine can't send a messages to a user]
>
>
> ppFactorySSHImpl
>
> * use ProtocolNames.SSH here again
>
> didn't get this
>
>
> ppFactorySSH
> * again, this class is a clever solution !
>
>
> ppServiceSSHImpl
> * use ProtocolNames.SSH here again
>
> didn't get this
>
> Resources (both)
>
> * getImage : the array has a fixed size, it's dirty and may
> cause some
> problem, why not use the InputStream.available to fix a
> dynamic size for
> the byte array ?
>
> Corrected
>
>
> resources.properties
> * Some parameters haven't their place here like the timeout
> value. To
> see where to put these parameters, take a look at what Yana do
> to save
> the last status of an account.
>
> * the testCommand and response aren't used, why keep it on
> this file ?
>
> These are temporary solutions to tricky problems .. a better soln
> needs to be found.
>
>
> SSHActivator
>
> * use ProtocolNames.SSH here again
>
> didn't get this
>
> SSHUserInfo
>
> * promptYesNo : The dialog type is "warning" however you're
> asking a
> question so why didn't you chose the "question" type ?
>
> Corrected
>
>
> resources.properties in the wizzard
>
> * use ProtocolNames.SSH here again for the protocol name
>
> didn't get this
>
> SSHAccountRegistration
>
> * identityFile : I dont understand what it is. Can you tell it
> in the
> javadoc ?
>
> Identity file is a private[default] key of the user which is one of
> the methods of authentication [its a standard in SSH]
>
>
> * Why the server port is a String and not an int in the
> methods ?
>
> same as in OpSetContactInfo
>
>
> And that's all.
> Once again, don't worry about this list, you've done a great
> job Jindal !
> However if you want your baby live in SC, you'll need to
> provide us a
> little patch which correct all this and especially the little
> security
> problem.
>
> Cheers,
> Ben
>
>
---------------------------------------------------------------------
> To unsubscribe, e-mail:
> dev-unsubscribe@sip-communicator.dev.java.net
> For additional commands, e-mail:
> dev-help@sip-communicator.dev.java.net
>
>
>
> --
> Shobhit Jindal
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
> For additional commands, e-mail: dev-help@sip-communicator.dev.java.net
--

(C) Matthew Rubenstein


#6

Hey Matthew,

The SSH feature simply allows using SIP Communicator for establishing
secure shell connections with SSH servers. It's a feature that is not
directly related to VoIP or instant messaging.

Emil

Matthew Rubenstein wrote:

路路路

聽聽Does this SSH feature in S-C work with the encryption in Asterisk
v1.4.x? If not, which SIP server does it work with? Or is it only for
P2P S-C sessions?

On Sat, 2007-10-27 at 01:09 +0530, Shobhit Jindal wrote:

Hi Benoit,

Sorry for the really long delay!

Thanks for reviewing my patch :slight_smile: .. I am attaching SSH protocol impl
and the wizard files with this mail which incorporate many changes as
described below..
(i tried to create a patch with svn diff but it didnt incorporate
newly created files and delete non existing ones)

On 9/28/07, Benoit Pradelle <ze_real_neo@yahoo.fr> wrote:
聽聽聽聽聽聽聽聽Hi all, Jindal,
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽I finally reviewed your ssh patch (which was really huge :)).
聽聽聽聽聽聽聽聽First I'd like to say that you've done a very good job, your
聽聽聽聽聽聽聽聽code is
聽聽聽聽聽聽聽聽clear, understandable and well structured and the amount of
聽聽聽聽聽聽聽聽work is
聽聽聽聽聽聽聽聽impressive.
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽However, I found a security problem during my review so I
聽聽聽聽聽聽聽聽decided to not
聽聽聽聽聽聽聽聽activate your code for the moment.
聽聽聽聽聽聽聽聽Here are the little things I've noticed on your code and which
聽聽聽聽聽聽聽聽need a
聽聽聽聽聽聽聽聽clarification or a correction :slight_smile: (don't worry about the amount
聽聽聽聽聽聽聽聽of
聽聽聽聽聽聽聽聽question, it's quite normal and it absolutely doesn't mean
聽聽聽聽聽聽聽聽that your
聽聽聽聽聽聽聽聽code has problems everywhere)
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* there were some javadoc / 80 characters deadline problems
聽聽聽聽聽聽聽聽and some are
聽聽聽聽聽聽聽聽still not fixed but as cruiseControl will probably cry, I'll
聽聽聽聽聽聽聽聽probably
聽聽聽聽聽聽聽聽correct them these days

I didnt get this :slight_smile: please explain

聽聽聽聽聽聽聽聽in ContactSSHFileTransferDaemon
聽聽聽聽聽聽聽聽* I don't really understand the name of this class. Why
聽聽聽聽聽聽聽聽contact ?
聽聽聽聽聽聽聽聽* constructor :

Corrected
In the start i tried to maintain a clear difference between classes
specific to ssh account and a contact.. its no more needed

聽聽聽聽聽聽聽聽public ContactSSHFileTransferDaemon(
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽ContactSSH sshContact,
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽OperationSetPersistentPresenceSSHImpl
聽聽聽聽聽聽聽聽opSetPersPresence,
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽OperationSetBasicInstantMessagingSSHImpl
聽聽聽聽聽聽聽聽instantMessaging,
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽ProtocolProviderServiceSSHImpl ppService)
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽I think that all the operationSets are deductibles from the
聽聽聽聽聽聽聽聽ppService,
聽聽聽聽聽聽聽聽aren't they ?

Corrected

聽聽聽聽聽聽聽聽* upload / dowload : is it possible to have many requests for
聽聽聽聽聽聽聽聽an upload
聽聽聽聽聽聽聽聽or a download in the same time ? If the answer is yes, your
聽聽聽聽聽聽聽聽code will
聽聽聽聽聽聽聽聽probably fail as you're using global variables to store the
聽聽聽聽聽聽聽聽parameters
聽聽聽聽聽聽聽聽of the operations

which global variables specifically?
A new instance of this daemon is created for each upload/download ..

simulataneous upload/downloads seem to be working fine

聽聽聽聽聽聽聽聽* you're display exception in the chat window, I think that
聽聽聽聽聽聽聽聽you're doing
聽聽聽聽聽聽聽聽the logger's job, you should probably just need to use the
聽聽聽聽聽聽聽聽logger for this

In case of any error I need to notify the user somehow ... giving a
generic error may not be of much use .. so am displaying the
exception to the user (its also logged as an error)

聽聽聽聽聽聽聽聽* line 408 : for(int i=0;;i++) <- very surprising
聽聽聽聽聽聽聽聽construction,
聽聽聽聽聽聽聽聽I didn't even know that a such construction was permitted :slight_smile:

lol :slight_smile: any better way to increment a variable for an infinite loop?

聽聽聽聽聽聽聽聽* line 437 you're treating an error case but you don't throw
聽聽聽聽聽聽聽聽an
聽聽聽聽聽聽聽聽exception here. Why ?
its a break condition .. (IO errors are automatically thrown by the
method in throws clause)

聽聽聽聽聽聽聽聽ContactSSH
聽聽聽聽聽聽聽聽I think that this specialized interface is a really good
聽聽聽聽聽聽聽聽idea !
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽ContactSSHReaderDaemon
聽聽聽聽聽聽聽聽* Why "contact" in this class name ? (again)

Corrected

聽聽聽聽聽聽聽聽* You're waiting 50ms for a message to come but, to me, it
聽聽聽聽聽聽聽聽seems very
聽聽聽聽聽聽聽聽CPU consuming. 500ms isn't sufficient ?

Reader daemon checks for any new messages every 50ms and then buffers
till 250ms if there is any.. and am sleeping instead of
spinlocking ..

聽聽聽聽聽聽聽聽* !!! big fat warning !!! (here is the security problem)
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽When you're reading the messages, you're putting in a buffer
聽聽聽聽聽聽聽聽everything
聽聽聽聽聽聽聽聽received until having 250ms during which nothing is
聽聽聽聽聽聽聽聽transmitted.
聽聽聽聽聽聽聽聽Now think a little bit on this :
聽聽聽聽聽聽聽聽Jay is really happy, SC now support SSH, he is very impatient
聽聽聽聽聽聽聽聽and wants
聽聽聽聽聽聽聽聽to test it NOW ! so he started his local server and, using his
聽聽聽聽聽聽聽聽brand new
聽聽聽聽聽聽聽聽Gigabyte network, he connects himself to his server. Now, to
聽聽聽聽聽聽聽聽test if
聽聽聽聽聽聽聽聽everything is ok, he types "cat /proc/random" and nothing
聽聽聽聽聽聽聽聽happens but a
聽聽聽聽聽聽聽聽few seconds later, his java VM crashes.
聽聽聽聽聽聽聽聽It's simply because, you added the server answer in the buffer
聽聽聽聽聽聽聽聽again and
聽聽聽聽聽聽聽聽again until reaching the end of the memory.

Agreed :slight_smile:

聽聽聽聽聽聽聽聽A simple fix for it is to set a limit to the buffer, when the
聽聽聽聽聽聽聽聽limit is
聽聽聽聽聽聽聽聽reached, simply display the content of the buffer until the
聽聽聽聽聽聽聽聽last '\n' to
聽聽聽聽聽聽聽聽keep a coherent display.
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽This MUST be corrected before activating SSH in SC.

Corrected .. a buffer limit of 32700 has been set.

聽聽聽聽聽聽聽聽MessageSSHImpl
聽聽聽聽聽聽聽聽* you consider null as an equivalent of plain/text - UTF8.
聽聽聽聽聽聽聽聽Isn't it a
聽聽聽聽聽聽聽聽little bit dangerous for the future developments ? (risks of
聽聽聽聽聽聽聽聽NullPointerException)

presently am not using/checking for content type of message
anywhere .. so its set to null

聽聽聽聽聽聽聽聽OpSetBasicIM
聽聽聽聽聽聽聽聽* like before, the opset can be deducted from the ppService,
聽聽聽聽聽聽聽聽so aren't
聽聽聽聽聽聽聽聽needed as parameters

Corrected

聽聽聽聽聽聽聽聽* isContentTypeSupported never returns true. Does it means
聽聽聽聽聽聽聽聽that you're
聽聽聽聽聽聽聽聽not able to send any sort of content ?

method is never called :slight_smile: as content type is of no relevance
presently

聽聽聽聽聽聽聽聽OpSetContactInfo
聽聽聽聽聽聽聽聽* There are no type verification especially for the port
聽聽聽聽聽聽聽聽number which
聽聽聽聽聽聽聽聽remains a String. Isn't it a little bit dangerous for other
聽聽聽聽聽聽聽聽classes
聽聽聽聽聽聽聽聽which will perhaps try to use it as an Integer ?

Corrected by making port text field accept only digits (max 5)
Am trying to maintain contact info at one place - in the GUI form
where port represented by a textfield ..

聽聽聽聽聽聽聽聽* Why OperationSet in the class name ?

Corrected

聽聽聽聽聽聽聽聽OpSetContactTimer
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* Why OperationSet in the class name ?

Corrected

聽聽聽聽聽聽聽聽OpSetFileTransfert
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* like before, some parameters in the constructor aren't
聽聽聽聽聽聽聽聽needed

Corrected

聽聽聽聽聽聽聽聽* assert false at the end ?

Presently am unsure of how will interfaces of generic file transfer
for SC will evolve ..

Hence am going for non-standard assumptions and making an
implementation specific for SSH File transfer...
there may be more than 2 modes of transfer in generic file transfer
but SSH one supports only 2 .. so asserting false at end may help...

聽聽聽聽聽聽聽聽* you use file.separator in this class and it's a very good
聽聽聽聽聽聽聽聽idea but
聽聽聽聽聽聽聽聽just after you use a dirty '/', why ?

Corrected :slight_smile:

聽聽聽聽聽聽聽聽OpSetPersistentPresence
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* subscribe :
聽聽聽聽聽聽聽聽sshContact.getSSHConfigurationForm().setVisible(true);
聽聽聽聽聽聽聽聽What's that ?

As soon as a contact is created[subscribed] .. the user must provide
server name/ip address of the remote machine before its added to
contacts list ...

The above makes the config form of remote machine visible in the
subsribe method..

聽聽聽聽聽聽聽聽* createUnresolvedContact : startTimerTask is perhaps not
聽聽聽聽聽聽聽聽needed as,
聽聽聽聽聽聽聽聽normally, an unresolved contact doesn't generate network
聽聽聽聽聽聽聽聽communications.

Contacts can only be resolved the Timer Task [update its status in
Contact list]
anyways timer task just checks for reachability [just an innocent
open/close on ssh port presently]

聽聽聽聽聽聽聽聽* use protocolNames for the name of the service in the Osgi
聽聽聽聽聽聽聽聽requests

I didnt get the above .. same for all protocolNames.SSH below

聽聽聽聽聽聽聽聽* why unresolvedContact have resolved=true and volatile
聽聽聽聽聽聽聽聽resolved=false ?

there are no volatile contacts in SSH .. this code is redundant
[an unknown/unspecified machine can't send a messages to a user]

聽聽聽聽聽聽聽聽ppFactorySSHImpl
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* use ProtocolNames.SSH here again

didn't get this

聽聽聽聽聽聽聽聽ppFactorySSH
聽聽聽聽聽聽聽聽* again, this class is a clever solution !
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽ppServiceSSHImpl
聽聽聽聽聽聽聽聽* use ProtocolNames.SSH here again

didn't get this

聽聽聽聽聽聽聽聽Resources (both)
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* getImage : the array has a fixed size, it's dirty and may
聽聽聽聽聽聽聽聽cause some
聽聽聽聽聽聽聽聽problem, why not use the InputStream.available to fix a
聽聽聽聽聽聽聽聽dynamic size for
聽聽聽聽聽聽聽聽the byte array ?

Corrected

聽聽聽聽聽聽聽聽resources.properties
聽聽聽聽聽聽聽聽* Some parameters haven't their place here like the timeout
聽聽聽聽聽聽聽聽value. To
聽聽聽聽聽聽聽聽see where to put these parameters, take a look at what Yana do
聽聽聽聽聽聽聽聽to save
聽聽聽聽聽聽聽聽the last status of an account.
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* the testCommand and response aren't used, why keep it on
聽聽聽聽聽聽聽聽this file ?

These are temporary solutions to tricky problems .. a better soln
needs to be found.

聽聽聽聽聽聽聽聽SSHActivator
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* use ProtocolNames.SSH here again

didn't get this

聽聽聽聽聽聽聽聽SSHUserInfo
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* promptYesNo : The dialog type is "warning" however you're
聽聽聽聽聽聽聽聽asking a
聽聽聽聽聽聽聽聽question so why didn't you chose the "question" type ?

Corrected

聽聽聽聽聽聽聽聽resources.properties in the wizzard
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* use ProtocolNames.SSH here again for the protocol name

didn't get this

聽聽聽聽聽聽聽聽SSHAccountRegistration
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽* identityFile : I dont understand what it is. Can you tell it
聽聽聽聽聽聽聽聽in the
聽聽聽聽聽聽聽聽javadoc ?

Identity file is a private[default] key of the user which is one of
the methods of authentication [its a standard in SSH]

聽聽聽聽聽聽聽聽* Why the server port is a String and not an int in the
聽聽聽聽聽聽聽聽methods ?

same as in OpSetContactInfo

聽聽聽聽聽聽聽聽And that's all.
聽聽聽聽聽聽聽聽Once again, don't worry about this list, you've done a great
聽聽聽聽聽聽聽聽job Jindal !
聽聽聽聽聽聽聽聽However if you want your baby live in SC, you'll need to
聽聽聽聽聽聽聽聽provide us a
聽聽聽聽聽聽聽聽little patch which correct all this and especially the little
聽聽聽聽聽聽聽聽security
聽聽聽聽聽聽聽聽problem.
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽Cheers,
聽聽聽聽聽聽聽聽Ben
聽聽聽聽聽聽聽聽
聽聽聽聽聽聽聽聽---------------------------------------------------------------------
聽聽聽聽聽聽聽聽To unsubscribe, e-mail:
聽聽聽聽聽聽聽聽dev-unsubscribe@sip-communicator.dev.java.net
聽聽聽聽聽聽聽聽For additional commands, e-mail:
聽聽聽聽聽聽聽聽dev-help@sip-communicator.dev.java.net
聽聽聽聽聽聽聽聽
--
Shobhit Jindal
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#7

Hi Jindal,

It's good to have some news of you :).
Thanks for your corrections but it'll be wonderful if you could send us a new zip the way you did it with the corrections considering my following answers !

Shobhit Jindal a 茅crit :

Hi Benoit,

Sorry for the really long delay!

Thanks for reviewing my patch :slight_smile: .. I am attaching SSH protocol impl and the wizard files with this mail which incorporate many changes as described below..
(i tried to create a patch with svn diff but it didnt incorporate newly created files and delete non existing ones)

聽聽聽聽Hi all, Jindal,

聽聽聽聽I finally reviewed your ssh patch (which was really huge :)).
聽聽聽聽First I'd like to say that you've done a very good job, your code is
聽聽聽聽clear, understandable and well structured and the amount of work is
聽聽聽聽impressive.

聽聽聽聽However, I found a security problem during my review so I decided
聽聽聽聽to not
聽聽聽聽activate your code for the moment.
聽聽聽聽Here are the little things I've noticed on your code and which need a
聽聽聽聽clarification or a correction :slight_smile: (don't worry about the amount of
聽聽聽聽question, it's quite normal and it absolutely doesn't mean that your
聽聽聽聽code has problems everywhere)

聽聽聽聽* there were some javadoc / 80 characters deadline problems and
聽聽聽聽some are
聽聽聽聽still not fixed but as cruiseControl will probably cry, I'll probably
聽聽聽聽correct them these days

I didnt get this :slight_smile: please explain

The developer documentation specifies a limit in the width of any source file, this limit is set at 80 characters meaning that every line with more than 80 characters must be divided in two or more lines.
When I talk about javadoc problem I mean some mistakes (wrong spell, missing parameter or return comment, ...). These mistakes in the javadoc make cruise control vomiting a lot of dirty warnings in the mailing list until we correct them.

聽聽聽聽in ContactSSHFileTransferDaemon
聽聽聽聽* I don't really understand the name of this class. Why contact ?
聽聽聽聽* constructor :

Corrected
In the start i tried to maintain a clear difference between classes specific to ssh account and a contact.. its no more needed

聽聽聽聽public ContactSSHFileTransferDaemon(
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽ContactSSH sshContact,
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽OperationSetPersistentPresenceSSHImpl opSetPersPresence,
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽OperationSetBasicInstantMessagingSSHImpl instantMessaging,
聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽聽ProtocolProviderServiceSSHImpl ppService)

聽聽聽聽I think that all the operationSets are deductibles from the
聽聽聽聽ppService,
聽聽聽聽aren't they ?

Corrected

聽聽聽聽* upload / dowload : is it possible to have many requests for an
聽聽聽聽upload
聽聽聽聽or a download in the same time ? If the answer is yes, your code will
聽聽聽聽probably fail as you're using global variables to store the parameters
聽聽聽聽of the operations

which global variables specifically?
A new instance of this daemon is created for each upload/download ..
simulataneous upload/downloads seem to be working fine

If a new instance is created for each transfert, everything must be ok.

聽聽聽聽* you're display exception in the chat window, I think that you're
聽聽聽聽doing
聽聽聽聽the logger's job, you should probably just need to use the logger
聽聽聽聽for this

In case of any error I need to notify the user somehow ... giving a generic error may not be of much use .. so am displaying the exception to the user (its also logged as an error)

That's right, it's probably the best solution here.

聽聽聽聽* line 408 : for(int i=0;;i++) <- very surprising
聽聽聽聽construction,
聽聽聽聽I didn't even know that a such construction was permitted :slight_smile:

lol :slight_smile: any better way to increment a variable for an infinite loop?

Perhaps adding a true as a condition will make this line a little bit more clear, no ?

聽聽聽聽* line 437 you're treating an error case but you don't throw an
聽聽聽聽exception here. Why ?

its a break condition .. (IO errors are automatically thrown by the method in throws clause)

Ok so there are two cases :
- first, as you say a read op always throws an exception when something bad occur, thus you're test is not necessary here as never reached.
- second in some error cases no exception is thrown, your test pass and... nothing warns the user !

In both case something has to be corrected and IMHO, the second case should be considered as it's the most general one.

聽聽聽聽ContactSSH
聽聽聽聽I think that this specialized interface is a really good idea !

聽聽聽聽ContactSSHReaderDaemon
聽聽聽聽* Why "contact" in this class name ? (again)

Corrected

聽聽聽聽* You're waiting 50ms for a message to come but, to me, it seems very
聽聽聽聽CPU consuming. 500ms isn't sufficient ?

Reader daemon checks for any new messages every 50ms and then buffers till 250ms if there is any.. and am sleeping instead of spinlocking ..

Yes I know all this but do you think that it's really necessary to wake up the thread every 50 ms ? This wake up is CPU consuming I think and having an half second precision is really acceptable I think or if you wan you can set a little bit less than 500 ms but 50ms seems really too much.
Am I clear ?

聽聽聽聽* !!! big fat warning !!! (here is the security problem)

聽聽聽聽When you're reading the messages, you're putting in a buffer
聽聽聽聽everything
聽聽聽聽received until having 250ms during which nothing is transmitted.
聽聽聽聽Now think a little bit on this :
聽聽聽聽Jay is really happy, SC now support SSH, he is very impatient and
聽聽聽聽wants
聽聽聽聽to test it NOW ! so he started his local server and, using his
聽聽聽聽brand new
聽聽聽聽Gigabyte network, he connects himself to his server. Now, to test if
聽聽聽聽everything is ok, he types "cat /proc/random" and nothing happens
聽聽聽聽but a
聽聽聽聽few seconds later, his java VM crashes.
聽聽聽聽It's simply because, you added the server answer in the buffer
聽聽聽聽again and
聽聽聽聽again until reaching the end of the memory.

Agreed :slight_smile:

聽聽聽聽A simple fix for it is to set a limit to the buffer, when the limit is
聽聽聽聽reached, simply display the content of the buffer until the last
聽聽聽聽'\n' to
聽聽聽聽keep a coherent display.

聽聽聽聽This MUST be corrected before activating SSH in SC.

Corrected .. a buffer limit of 32700 has been set.

I can't resist to bother you a little bit more and ask you to set a power of two limit as this limit is arbitrary and every power of two memory areas can save some bytes on the user's computer.
It's really a detail but I'm sure that in big projects like SC, these sort of little optimizations are really making a difference :slight_smile: (please don't troll me).

聽聽聽聽MessageSSHImpl
聽聽聽聽* you consider null as an equivalent of plain/text - UTF8. Isn't it a
聽聽聽聽little bit dangerous for the future developments ? (risks of
聽聽聽聽NullPointerException)

presently am not using/checking for content type of message anywhere .. so its set to null

Yes but it's not unbelievable that one day, you or another one use this without knowing this specificity of your code and create some dirty NullPointerException, is it ?

聽聽聽聽OpSetBasicIM
聽聽聽聽* like before, the opset can be deducted from the ppService, so
聽聽聽聽aren't
聽聽聽聽needed as parameters

Corrected

聽聽聽聽* isContentTypeSupported never returns true. Does it means that
聽聽聽聽you're
聽聽聽聽not able to send any sort of content ?

method is never called :slight_smile: as content type is of no relevance presently

As I said before, of course it's ok for now but if one day we need to implement something that has to know what are all the supported content-types or something like this, your method will not be really revelant.

聽聽聽聽OpSetContactInfo
聽聽聽聽* There are no type verification especially for the port number which
聽聽聽聽remains a String. Isn't it a little bit dangerous for other classes
聽聽聽聽which will perhaps try to use it as an Integer ?

Corrected by making port text field accept only digits (max 5)
Am trying to maintain contact info at one place - in the GUI form where port represented by a textfield ..

聽聽聽聽* Why OperationSet in the class name ?

Corrected

聽聽聽聽OpSetContactTimer

聽聽聽聽* Why OperationSet in the class name ?

Corrected

聽聽聽聽OpSetFileTransfert

聽聽聽聽* like before, some parameters in the constructor aren't needed

Corrected

聽聽聽聽* assert false at the end ?

Presently am unsure of how will interfaces of generic file transfer for SC will evolve ..
Hence am going for non-standard assumptions and making an implementation specific for SSH File transfer...
there may be more than 2 modes of transfer in generic file transfer but SSH one supports only 2 .. so asserting false at end may help...

Ok I understand this but "assert false" isn't a valid java command so the compilation will fail if something like this remains in the code and I don't think that it's what you want.

聽聽聽聽* you use file.separator in this class and it's a very good idea but
聽聽聽聽just after you use a dirty '/', why ?

Corrected :slight_smile:

聽聽聽聽OpSetPersistentPresence

聽聽聽聽* subscribe : sshContact.getSSHConfigurationForm().setVisible(true);
聽聽聽聽What's that ?

As soon as a contact is created[subscribed] .. the user must provide server name/ip address of the remote machine before its added to contacts list ...

The above makes the config form of remote machine visible in the subsribe method..

Isn't this the role of the account creation wizzard ?

聽聽聽聽* createUnresolvedContact : startTimerTask is perhaps not needed as,
聽聽聽聽normally, an unresolved contact doesn't generate network
聽聽聽聽communications.

Contacts can only be resolved the Timer Task [update its status in Contact list] anyways timer task just checks for reachability [just an innocent open/close on ssh port presently]

I don't know of what to do of this but to me it seems that having a network connection in this method is not valid considering the javadoc.

聽聽聽聽* use protocolNames for the name of the service in the Osgi requests

I didnt get the above .. same for all protocolNames.SSH below

There is a class nammed protocolNames which role is to contain any protocol name. It's better to use this than using dirty String. You can add a value for the SSH protocol in it and use it.

聽聽聽聽* why unresolvedContact have resolved=true and volatile
聽聽聽聽resolved=false ?

there are no volatile contacts in SSH .. this code is redundant
[an unknown/unspecified machine can't send a messages to a user]

I just wanted to point the incoherence here as SSH unresolved contacts have resolved = true while they are by definition unresolved and volatile ones have resolved=false, to me, it looks a little bit strange.

聽聽聽聽ppFactorySSHImpl

聽聽聽聽* use ProtocolNames.SSH here again

didn't get this

聽聽聽聽ppFactorySSH
聽聽聽聽* again, this class is a clever solution !

聽聽聽聽ppServiceSSHImpl
聽聽聽聽* use ProtocolNames.SSH here again

didn't get this

聽聽聽聽Resources (both)

聽聽聽聽* getImage : the array has a fixed size, it's dirty and may cause some
聽聽聽聽problem, why not use the InputStream.available to fix a dynamic
聽聽聽聽size for
聽聽聽聽the byte array ?

Corrected

聽聽聽聽resources.properties
聽聽聽聽* Some parameters haven't their place here like the timeout value. To
聽聽聽聽see where to put these parameters, take a look at what Yana do to save
聽聽聽聽the last status of an account.

What about this ?

聽聽聽聽* the testCommand and response aren't used, why keep it on this file ?

These are temporary solutions to tricky problems .. a better soln needs to be found.

If they are unused, why keeping them stored in a file ?

聽聽聽聽SSHActivator

聽聽聽聽* use ProtocolNames.SSH here again

didn't get this

聽聽聽聽SSHUserInfo

聽聽聽聽* promptYesNo : The dialog type is "warning" however you're asking a
聽聽聽聽question so why didn't you chose the "question" type ?

Corrected

聽聽聽聽resources.properties in the wizzard

聽聽聽聽* use ProtocolNames.SSH here again for the protocol name

didn't get this

聽聽聽聽SSHAccountRegistration

聽聽聽聽* identityFile : I dont understand what it is. Can you tell it in the
聽聽聽聽javadoc ?

Identity file is a private[default] key of the user which is one of the methods of authentication [its a standard in SSH]

Please add this in javadoc :slight_smile:

聽聽聽聽* Why the server port is a String and not an int in the methods ?

same as in OpSetContactInfo

聽聽聽聽And that's all.
聽聽聽聽Once again, don't worry about this list, you've done a great job
聽聽聽聽Jindal !
聽聽聽聽However if you want your baby live in SC, you'll need to provide us a
聽聽聽聽little patch which correct all this and especially the little security
聽聽聽聽problem.

聽聽聽聽Cheers,
聽聽聽聽Ben

聽聽聽聽---------------------------------------------------------------------
聽聽聽聽To unsubscribe, e-mail:
聽聽聽聽dev-unsubscribe@sip-communicator.dev.java.net
聽聽聽聽<mailto:dev-unsubscribe@sip-communicator.dev.java.net>
聽聽聽聽For additional commands, e-mail:
聽聽聽聽dev-help@sip-communicator.dev.java.net
聽聽聽聽<mailto:dev-help@sip-communicator.dev.java.net>

--
Shobhit Jindal
------------------------------------------------------------------------

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

Here we are. Thanks for your first corrections, I'll wait your next patch to commit all these correction in one patch.

Cheers,
Ben.

路路路

On 9/28/07, *Benoit Pradelle* <ze_real_neo@yahoo.fr > <mailto:ze_real_neo@yahoo.fr>> wrote:

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#8

Hi Benoit, All

No excuses for being late :frowning:

please find the replies inline and latest sources attached as a zip.

Hi Jindal,

It's good to have some news of you :).
Thanks for your corrections but it'll be wonderful if you could send us
a new zip the way you did it with the corrections considering my
following answers !

Hopefully I will more regular from now on :slight_smile:

Shobhit Jindal a 茅crit :
> Hi Benoit,
>
> Sorry for the really long delay!
>
> Thanks for reviewing my patch :slight_smile: .. I am attaching SSH protocol impl
> and the wizard files with this mail which incorporate many changes as
> described below..
> (i tried to create a patch with svn diff but it didnt incorporate
> newly created files and delete non existing ones)
>
>
> Hi all, Jindal,
>
> I finally reviewed your ssh patch (which was really huge :)).
> First I'd like to say that you've done a very good job, your code is
> clear, understandable and well structured and the amount of work is
> impressive.
>
> However, I found a security problem during my review so I decided
> to not
> activate your code for the moment.
> Here are the little things I've noticed on your code and which need
a
> clarification or a correction :slight_smile: (don't worry about the amount of
> question, it's quite normal and it absolutely doesn't mean that your
> code has problems everywhere)
>
> * there were some javadoc / 80 characters deadline problems and
> some are
> still not fixed but as cruiseControl will probably cry, I'll
probably
> correct them these days
>
>
> I didnt get this :slight_smile: please explain
The developer documentation specifies a limit in the width of any source
file, this limit is set at 80 characters meaning that every line with
more than 80 characters must be divided in two or more lines.
When I talk about javadoc problem I mean some mistakes (wrong spell,
missing parameter or return comment, ...). These mistakes in the javadoc
make cruise control vomiting a lot of dirty warnings in the mailing list
until we correct them.

Agreed, I have corrected most of these.

>
> in ContactSSHFileTransferDaemon
> * I don't really understand the name of this class. Why contact ?
> * constructor :
>
>
> Corrected
> In the start i tried to maintain a clear difference between classes
> specific to ssh account and a contact.. its no more needed
>
> public ContactSSHFileTransferDaemon(
> ContactSSH sshContact,
> OperationSetPersistentPresenceSSHImpl opSetPersPresence,

> OperationSetBasicInstantMessagingSSHImpl
instantMessaging,
> ProtocolProviderServiceSSHImpl ppService)
>
> I think that all the operationSets are deductibles from the
> ppService,
> aren't they ?
>
>
> Corrected
>
> * upload / dowload : is it possible to have many requests for an
> upload
> or a download in the same time ? If the answer is yes, your code
will
> probably fail as you're using global variables to store the
parameters
> of the operations
>
>
> which global variables specifically?
> A new instance of this daemon is created for each upload/download ..
> simulataneous upload/downloads seem to be working fine

If a new instance is created for each transfert, everything must be ok.

>
> * you're display exception in the chat window, I think that you're
> doing
> the logger's job, you should probably just need to use the logger
> for this
>
>
> In case of any error I need to notify the user somehow ... giving a
> generic error may not be of much use .. so am displaying the
> exception to the user (its also logged as an error)

That's right, it's probably the best solution here.

>
> * line 408 : for(int i=0;;i++) <- very surprising
> construction,
> I didn't even know that a such construction was permitted :slight_smile:
>
>
> lol :slight_smile: any better way to increment a variable for an infinite loop?

Perhaps adding a true as a condition will make this line a little bit
more clear, no ?

Corrected!

>
> * line 437 you're treating an error case but you don't throw an
> exception here. Why ?
>
> its a break condition .. (IO errors are automatically thrown by the
> method in throws clause)

Ok so there are two cases :
- first, as you say a read op always throws an exception when something
bad occur, thus you're test is not necessary here as never reached.
- second in some error cases no exception is thrown, your test pass
and... nothing warns the user !

In both case something has to be corrected and IMHO, the second case
should be considered as it's the most general one.

The break signifies the end of file NOT an exception.

>
> ContactSSH
> I think that this specialized interface is a really good idea !
>
>
> ContactSSHReaderDaemon
> * Why "contact" in this class name ? (again)
>
>
> Corrected
>
> * You're waiting 50ms for a message to come but, to me, it seems
very
> CPU consuming. 500ms isn't sufficient ?
>
>
> Reader daemon checks for any new messages every 50ms and then buffers
> till 250ms if there is any.. and am sleeping instead of spinlocking ..

Yes I know all this but do you think that it's really necessary to wake
up the thread every 50 ms ? This wake up is CPU consuming I think and
having an half second precision is really acceptable I think or if you
wan you can set a little bit less than 500 ms but 50ms seems really too
much.
Am I clear ?

Got it.
500ms gave a sluggish feel, its 250ms presently.

This Reader Daemon for the remote machine seems to be more like a fix than a
nice soln.
Please see if ya can suggest some different approach to read the terminal.

>
> * !!! big fat warning !!! (here is the security problem)
>
> When you're reading the messages, you're putting in a buffer
> everything
> received until having 250ms during which nothing is transmitted.
> Now think a little bit on this :
> Jay is really happy, SC now support SSH, he is very impatient and
> wants
> to test it NOW ! so he started his local server and, using his
> brand new
> Gigabyte network, he connects himself to his server. Now, to test if
> everything is ok, he types "cat /proc/random" and nothing happens
> but a
> few seconds later, his java VM crashes.
> It's simply because, you added the server answer in the buffer
> again and
> again until reaching the end of the memory.
>
>
> Agreed :slight_smile:
>
> A simple fix for it is to set a limit to the buffer, when the limit
is
> reached, simply display the content of the buffer until the last
> '\n' to
> keep a coherent display.
>
> This MUST be corrected before activating SSH in SC.
>
>
> Corrected .. a buffer limit of 32700 has been set.

I can't resist to bother you a little bit more and ask you to set a
power of two limit as this limit is arbitrary and every power of two
memory areas can save some bytes on the user's computer.
It's really a detail but I'm sure that in big projects like SC, these
sort of little optimizations are really making a difference :slight_smile: (please
don't troll me).

Agreed! Its now set to 16384.

>
> MessageSSHImpl
> * you consider null as an equivalent of plain/text - UTF8. Isn't it
a
> little bit dangerous for the future developments ? (risks of
> NullPointerException)
>
>
> presently am not using/checking for content type of message anywhere
> .. so its set to null

Yes but it's not unbelievable that one day, you or another one use this
without knowing this specificity of your code and create some dirty
NullPointerException, is it ?

Agreed! its set to text/plain.

>
> OpSetBasicIM
> * like before, the opset can be deducted from the ppService, so
> aren't
> needed as parameters
>
>
> Corrected
>
> * isContentTypeSupported never returns true. Does it means that
> you're
> not able to send any sort of content ?
>
>
> method is never called :slight_smile: as content type is of no relevance presently

As I said before, of course it's ok for now but if one day we need to
implement something that has to know what are all the supported
content-types or something like this, your method will not be really
revelant.

Agreed it compares it content type to the default content type (text/plain)
for Message

>
> OpSetContactInfo
> * There are no type verification especially for the port number
which
> remains a String. Isn't it a little bit dangerous for other classes
> which will perhaps try to use it as an Integer ?
>
>
> Corrected by making port text field accept only digits (max 5)
> Am trying to maintain contact info at one place - in the GUI form
> where port represented by a textfield ..
>
> * Why OperationSet in the class name ?
>
>
> Corrected
>
> OpSetContactTimer
>
> * Why OperationSet in the class name ?
>
>
> Corrected
>
> OpSetFileTransfert
>
> * like before, some parameters in the constructor aren't needed
>
>
> Corrected
>
> * assert false at the end ?
>
>
> Presently am unsure of how will interfaces of generic file transfer
> for SC will evolve ..
> Hence am going for non-standard assumptions and making an
> implementation specific for SSH File transfer...
> there may be more than 2 modes of transfer in generic file transfer
> but SSH one supports only 2 .. so asserting false at end may help...

Ok I understand this but "assert false" isn't a valid java command so
the compilation will fail if something like this remains in the code and
I don't think that it's what you want.

Okie, assert false is commented out. I now just log it as an error.

>
> * you use file.separator in this class and it's a very good idea but

> just after you use a dirty '/', why ?
>
>
> Corrected :slight_smile:
>
> OpSetPersistentPresence
>
> * subscribe : sshContact.getSSHConfigurationForm().setVisible(true);

> What's that ?
>
>
> As soon as a contact is created[subscribed] .. the user must provide
> server name/ip address of the remote machine before its added to
> contacts list ...
>
> The above makes the config form of remote machine visible in the
> subsribe method..

Isn't this the role of the account creation wizzard ?

No. Each contact needs to have its own info( server, account etc)

>
> * createUnresolvedContact : startTimerTask is perhaps not needed as,
> normally, an unresolved contact doesn't generate network
> communications.
>
>
> Contacts can only be resolved the Timer Task [update its status in
> Contact list]
> anyways timer task just checks for reachability [just an innocent
> open/close on ssh port presently]

I don't know of what to do of this but to me it seems that having a
network connection in this method is not valid considering the javadoc.

we need to have some network communication to get the status of the machine.
As Java 1.5 is in, I am using InetAddress.isReachable() method now.
the javadoc stands corrected.

>
> * use protocolNames for the name of the service in the Osgi requests
>
>
> I didnt get the above .. same for all protocolNames.SSH below

There is a class nammed protocolNames which role is to contain any
protocol name. It's better to use this than using dirty String. You can
add a value for the SSH protocol in it and use it.

Agreed! I wasn't aware of this earlier

>
> * why unresolvedContact have resolved=true and volatile
> resolved=false ?
>
>
> there are no volatile contacts in SSH .. this code is redundant
> [an unknown/unspecified machine can't send a messages to a user]

I just wanted to point the incoherence here as SSH unresolved contacts
have resolved = true while they are by definition unresolved and
volatile ones have resolved=false, to me, it looks a little bit strange.

Agreed, it does seem strange.

My understanding is that SSH contacts don't appear on their own like
volatile contacts (so volatile resolved is set to false)
and as user manually adds each contact, they are automatically resolved.

Please correct me.

>
> ppFactorySSHImpl
>
> * use ProtocolNames.SSH here again
>
>

Corrected!

> didn't get this
>
> ppFactorySSH
> * again, this class is a clever solution !
>
>
> ppServiceSSHImpl
> * use ProtocolNames.SSH here again
>
>

Corrected!

> didn't get this
>
> Resources (both)
>
> * getImage : the array has a fixed size, it's dirty and may cause
some
> problem, why not use the InputStream.available to fix a dynamic
> size for
> the byte array ?
>
>
> Corrected
>
> resources.properties
> * Some parameters haven't their place here like the timeout value.
To
> see where to put these parameters, take a look at what Yana do to
save
> the last status of an account.
>
What about this ?

Extra parameters have been removed.

>
> * the testCommand and response aren't used, why keep it on this file
?
>
>
> These are temporary solutions to tricky problems .. a better soln
> needs to be found.

If they are unused, why keeping them stored in a file ?

Extra parameters have been removed.

>
> SSHActivator
>
> * use ProtocolNames.SSH here again
>
>
> didn't get this
>
> SSHUserInfo
>
> * promptYesNo : The dialog type is "warning" however you're asking a

> question so why didn't you chose the "question" type ?
>
>
> Corrected
>
> resources.properties in the wizzard
>
> * use ProtocolNames.SSH here again for the protocol name
>
>
> didn't get this
>
> SSHAccountRegistration
>
> * identityFile : I dont understand what it is. Can you tell it in
the
> javadoc ?
>
>
> Identity file is a private[default] key of the user which is one of
> the methods of authentication [its a standard in SSH]

Please add this in javadoc :slight_smile:

Added :slight_smile:

ssh_6_1_08.zip (295 KB)

路路路

On Oct 27, 2007 8:55 PM, Benoit Pradelle < ze_real_neo@yahoo.fr> wrote:

> On 9/28/07, *Benoit Pradelle* < ze_real_neo@yahoo.fr > > <mailto:ze_real_neo@yahoo.fr>> wrote:

>
> * Why the server port is a String and not an int in the methods ?
>
>
> same as in OpSetContactInfo
>
> And that's all.
> Once again, don't worry about this list, you've done a great job
> Jindal !
> However if you want your baby live in SC, you'll need to provide us
a
> little patch which correct all this and especially the little
security
> problem.
>
> Cheers,
> Ben
>
>
---------------------------------------------------------------------
> To unsubscribe, e-mail:
> dev-unsubscribe@sip-communicator.dev.java.net
> <mailto:dev-unsubscribe@sip-communicator.dev.java.net>
> For additional commands, e-mail:
> dev-help@sip-communicator.dev.java.net
> <mailto:dev-help@sip-communicator.dev.java.net >
>
>
>
> --
> Shobhit Jindal
> ------------------------------------------------------------------------
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
> For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

Here we are. Thanks for your first corrections, I'll wait your next
patch to commit all these correction in one patch.

Cheers,
Ben.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

--
Shobhit Jindal
B.Tech. Part-IV,
Department Of Electronics Engineering,
Centre of Advanced Studies,
Institute Of Technology, BHU


#9

Hi Shobhit,

Thanks for your answer, I'm a little bit busy these days but I'll import your modification and activate SSH in SC as soon as possible.

Cheers,
Ben

Shobhit Jindal a 茅crit :

路路路

Hi Benoit, All

No excuses for being late :frowning:

please find the replies inline and latest sources attached as a zip.

On Oct 27, 2007 8:55 PM, Benoit Pradelle < ze_real_neo@yahoo.fr > <mailto:ze_real_neo@yahoo.fr>> wrote:

聽聽聽聽Hi Jindal,

聽聽聽聽It's good to have some news of you :).
聽聽聽聽Thanks for your corrections but it'll be wonderful if you could
聽聽聽聽send us
聽聽聽聽a new zip the way you did it with the corrections considering my
聽聽聽聽following answers !

Hopefully I will more regular from now on :slight_smile:

聽聽聽聽Shobhit Jindal a 茅crit :
聽聽聽聽> Hi Benoit,
聽聽聽聽>
聽聽聽聽> Sorry for the really long delay!
聽聽聽聽>
聽聽聽聽> Thanks for reviewing my patch :slight_smile: .. I am attaching SSH
聽聽聽聽protocol impl
聽聽聽聽> and the wizard files with this mail which incorporate many
聽聽聽聽changes as
聽聽聽聽> described below..
聽聽聽聽> (i tried to create a patch with svn diff but it didnt incorporate
聽聽聽聽> newly created files and delete non existing ones)
聽聽聽聽>
聽聽聽聽> On 9/28/07, *Benoit Pradelle* < ze_real_neo@yahoo.fr > <mailto:ze_real_neo@yahoo.fr> > > <mailto:ze_real_neo@yahoo.fr <mailto:ze_real_neo@yahoo.fr>>> wrote:
聽聽聽聽>
聽聽聽聽> Hi all, Jindal,
聽聽聽聽>
聽聽聽聽> I finally reviewed your ssh patch (which was really huge :)).
聽聽聽聽> First I'd like to say that you've done a very good job, your
聽聽聽聽code is
聽聽聽聽> clear, understandable and well structured and the amount of
聽聽聽聽work is
聽聽聽聽> impressive.
聽聽聽聽>
聽聽聽聽> However, I found a security problem during my review so I
聽聽聽聽decided
聽聽聽聽> to not
聽聽聽聽> activate your code for the moment.
聽聽聽聽> Here are the little things I've noticed on your code and
聽聽聽聽which need a
聽聽聽聽> clarification or a correction :slight_smile: (don't worry about the
聽聽聽聽amount of
聽聽聽聽> question, it's quite normal and it absolutely doesn't mean
聽聽聽聽that your
聽聽聽聽> code has problems everywhere)
聽聽聽聽>
聽聽聽聽> * there were some javadoc / 80 characters deadline problems and
聽聽聽聽> some are
聽聽聽聽> still not fixed but as cruiseControl will probably cry, I'll
聽聽聽聽probably
聽聽聽聽> correct them these days
聽聽聽聽>
聽聽聽聽> I didnt get this :slight_smile: please explain
聽聽聽聽The developer documentation specifies a limit in the width of any
聽聽聽聽source
聽聽聽聽file, this limit is set at 80 characters meaning that every line with
聽聽聽聽more than 80 characters must be divided in two or more lines.
聽聽聽聽When I talk about javadoc problem I mean some mistakes (wrong spell,
聽聽聽聽missing parameter or return comment, ...). These mistakes in the
聽聽聽聽javadoc
聽聽聽聽make cruise control vomiting a lot of dirty warnings in the
聽聽聽聽mailing list
聽聽聽聽until we correct them.

Agreed, I have corrected most of these.

聽聽聽聽>
聽聽聽聽> in ContactSSHFileTransferDaemon
聽聽聽聽> * I don't really understand the name of this class. Why
聽聽聽聽contact ?
聽聽聽聽> * constructor :
聽聽聽聽>
聽聽聽聽> Corrected
聽聽聽聽> In the start i tried to maintain a clear difference between classes
聽聽聽聽> specific to ssh account and a contact.. its no more needed
聽聽聽聽>
聽聽聽聽> public ContactSSHFileTransferDaemon(
聽聽聽聽> ContactSSH sshContact,
聽聽聽聽> OperationSetPersistentPresenceSSHImpl
聽聽聽聽opSetPersPresence,
聽聽聽聽> OperationSetBasicInstantMessagingSSHImpl
聽聽聽聽instantMessaging,
聽聽聽聽> ProtocolProviderServiceSSHImpl ppService)
聽聽聽聽>
聽聽聽聽> I think that all the operationSets are deductibles from the
聽聽聽聽> ppService,
聽聽聽聽> aren't they ?
聽聽聽聽>
聽聽聽聽> Corrected
聽聽聽聽>
聽聽聽聽> * upload / dowload : is it possible to have many requests for an
聽聽聽聽> upload
聽聽聽聽> or a download in the same time ? If the answer is yes, your
聽聽聽聽code will
聽聽聽聽> probably fail as you're using global variables to store the
聽聽聽聽parameters
聽聽聽聽> of the operations
聽聽聽聽>
聽聽聽聽> which global variables specifically?
聽聽聽聽> A new instance of this daemon is created for each
聽聽聽聽upload/download ..
聽聽聽聽> simulataneous upload/downloads seem to be working fine

聽聽聽聽If a new instance is created for each transfert, everything must
聽聽聽聽be ok.

聽聽聽聽>
聽聽聽聽> * you're display exception in the chat window, I think that
聽聽聽聽you're
聽聽聽聽> doing
聽聽聽聽> the logger's job, you should probably just need to use the
聽聽聽聽logger
聽聽聽聽> for this
聽聽聽聽>
聽聽聽聽> In case of any error I need to notify the user somehow ... giving a
聽聽聽聽> generic error may not be of much use .. so am displaying the
聽聽聽聽> exception to the user (its also logged as an error)

聽聽聽聽That's right, it's probably the best solution here.

聽聽聽聽>
聽聽聽聽> * line 408 : for(int i=0;;i++) <- very surprising
聽聽聽聽> construction,
聽聽聽聽> I didn't even know that a such construction was permitted :slight_smile:
聽聽聽聽>
聽聽聽聽> lol :slight_smile: any better way to increment a variable for an infinite loop?

聽聽聽聽Perhaps adding a true as a condition will make this line a little bit
聽聽聽聽more clear, no ?

Corrected!

聽聽聽聽>
聽聽聽聽> * line 437 you're treating an error case but you don't throw an
聽聽聽聽> exception here. Why ?
聽聽聽聽>
聽聽聽聽> its a break condition .. (IO errors are automatically thrown by the
聽聽聽聽> method in throws clause)

聽聽聽聽Ok so there are two cases :
聽聽聽聽- first, as you say a read op always throws an exception when
聽聽聽聽something
聽聽聽聽bad occur, thus you're test is not necessary here as never reached.
聽聽聽聽- second in some error cases no exception is thrown, your test pass
聽聽聽聽and... nothing warns the user !

聽聽聽聽In both case something has to be corrected and IMHO, the second case
聽聽聽聽should be considered as it's the most general one.

The break signifies the end of file NOT an exception.

聽聽聽聽>
聽聽聽聽> ContactSSH
聽聽聽聽> I think that this specialized interface is a really good idea !
聽聽聽聽>
聽聽聽聽> ContactSSHReaderDaemon
聽聽聽聽> * Why "contact" in this class name ? (again)
聽聽聽聽>
聽聽聽聽> Corrected
聽聽聽聽>
聽聽聽聽> * You're waiting 50ms for a message to come but, to me, it
聽聽聽聽seems very
聽聽聽聽> CPU consuming. 500ms isn't sufficient ?
聽聽聽聽>
聽聽聽聽> Reader daemon checks for any new messages every 50ms and then
聽聽聽聽buffers
聽聽聽聽> till 250ms if there is any.. and am sleeping instead of
聽聽聽聽spinlocking ..

聽聽聽聽Yes I know all this but do you think that it's really necessary to
聽聽聽聽wake
聽聽聽聽up the thread every 50 ms ? This wake up is CPU consuming I think and
聽聽聽聽having an half second precision is really acceptable I think or if you
聽聽聽聽wan you can set a little bit less than 500 ms but 50ms seems
聽聽聽聽really too
聽聽聽聽much.
聽聽聽聽Am I clear ?

Got it.
500ms gave a sluggish feel, its 250ms presently.

This Reader Daemon for the remote machine seems to be more like a fix than a nice soln.
Please see if ya can suggest some different approach to read the terminal.

聽聽聽聽>
聽聽聽聽> * !!! big fat warning !!! (here is the security problem)
聽聽聽聽>
聽聽聽聽> When you're reading the messages, you're putting in a buffer
聽聽聽聽> everything
聽聽聽聽> received until having 250ms during which nothing is
聽聽聽聽transmitted.
聽聽聽聽> Now think a little bit on this :
聽聽聽聽> Jay is really happy, SC now support SSH, he is very
聽聽聽聽impatient and
聽聽聽聽> wants
聽聽聽聽> to test it NOW ! so he started his local server and, using his
聽聽聽聽> brand new
聽聽聽聽> Gigabyte network, he connects himself to his server. Now, to
聽聽聽聽test if
聽聽聽聽> everything is ok, he types "cat /proc/random" and nothing
聽聽聽聽happens
聽聽聽聽> but a
聽聽聽聽> few seconds later, his java VM crashes.
聽聽聽聽> It's simply because, you added the server answer in the buffer
聽聽聽聽> again and
聽聽聽聽> again until reaching the end of the memory.
聽聽聽聽>
聽聽聽聽> Agreed :slight_smile:
聽聽聽聽>
聽聽聽聽> A simple fix for it is to set a limit to the buffer, when
聽聽聽聽the limit is
聽聽聽聽> reached, simply display the content of the buffer until the last
聽聽聽聽> '\n' to
聽聽聽聽> keep a coherent display.
聽聽聽聽>
聽聽聽聽> This MUST be corrected before activating SSH in SC.
聽聽聽聽>
聽聽聽聽> Corrected .. a buffer limit of 32700 has been set.

聽聽聽聽I can't resist to bother you a little bit more and ask you to set a
聽聽聽聽power of two limit as this limit is arbitrary and every power of two
聽聽聽聽memory areas can save some bytes on the user's computer.
聽聽聽聽It's really a detail but I'm sure that in big projects like SC, these
聽聽聽聽sort of little optimizations are really making a difference :slight_smile: (please
聽聽聽聽don't troll me).

Agreed! Its now set to 16384.

聽聽聽聽>
聽聽聽聽> MessageSSHImpl
聽聽聽聽> * you consider null as an equivalent of plain/text - UTF8.
聽聽聽聽Isn't it a
聽聽聽聽> little bit dangerous for the future developments ? (risks of
聽聽聽聽> NullPointerException)
聽聽聽聽>
聽聽聽聽> presently am not using/checking for content type of message anywhere
聽聽聽聽> .. so its set to null

聽聽聽聽Yes but it's not unbelievable that one day, you or another one use
聽聽聽聽this
聽聽聽聽without knowing this specificity of your code and create some dirty
聽聽聽聽NullPointerException, is it ?

Agreed! its set to text/plain.

聽聽聽聽>
聽聽聽聽> OpSetBasicIM
聽聽聽聽> * like before, the opset can be deducted from the ppService, so
聽聽聽聽> aren't
聽聽聽聽> needed as parameters
聽聽聽聽>
聽聽聽聽> Corrected
聽聽聽聽>
聽聽聽聽> * isContentTypeSupported never returns true. Does it means that
聽聽聽聽> you're
聽聽聽聽> not able to send any sort of content ?
聽聽聽聽>
聽聽聽聽> method is never called :slight_smile: as content type is of no relevance
聽聽聽聽presently

聽聽聽聽As I said before, of course it's ok for now but if one day we need to
聽聽聽聽implement something that has to know what are all the supported
聽聽聽聽content-types or something like this, your method will not be really
聽聽聽聽revelant.

Agreed it compares it content type to the default content type (text/plain) for Message

聽聽聽聽>
聽聽聽聽> OpSetContactInfo
聽聽聽聽> * There are no type verification especially for the port
聽聽聽聽number which
聽聽聽聽> remains a String. Isn't it a little bit dangerous for other
聽聽聽聽classes
聽聽聽聽> which will perhaps try to use it as an Integer ?
聽聽聽聽>
聽聽聽聽> Corrected by making port text field accept only digits (max 5)
聽聽聽聽> Am trying to maintain contact info at one place - in the GUI form
聽聽聽聽> where port represented by a textfield ..
聽聽聽聽>
聽聽聽聽> * Why OperationSet in the class name ?
聽聽聽聽>
聽聽聽聽> Corrected
聽聽聽聽>
聽聽聽聽> OpSetContactTimer
聽聽聽聽>
聽聽聽聽> * Why OperationSet in the class name ?
聽聽聽聽>
聽聽聽聽> Corrected
聽聽聽聽>
聽聽聽聽> OpSetFileTransfert
聽聽聽聽>
聽聽聽聽> * like before, some parameters in the constructor aren't needed
聽聽聽聽>
聽聽聽聽> Corrected
聽聽聽聽>
聽聽聽聽> * assert false at the end ?
聽聽聽聽>
聽聽聽聽> Presently am unsure of how will interfaces of generic file transfer
聽聽聽聽> for SC will evolve ..
聽聽聽聽> Hence am going for non-standard assumptions and making an
聽聽聽聽> implementation specific for SSH File transfer...
聽聽聽聽> there may be more than 2 modes of transfer in generic file transfer
聽聽聽聽> but SSH one supports only 2 .. so asserting false at end may help...

聽聽聽聽Ok I understand this but "assert false" isn't a valid java command so
聽聽聽聽the compilation will fail if something like this remains in the
聽聽聽聽code and
聽聽聽聽I don't think that it's what you want.

Okie, assert false is commented out. I now just log it as an error.

聽聽聽聽>
聽聽聽聽> * you use file.separator in this class and it's a very good
聽聽聽聽idea but
聽聽聽聽> just after you use a dirty '/', why ?
聽聽聽聽>
聽聽聽聽> Corrected :slight_smile:
聽聽聽聽>
聽聽聽聽> OpSetPersistentPresence
聽聽聽聽>
聽聽聽聽> * subscribe :
聽聽聽聽sshContact.getSSHConfigurationForm().setVisible(true);
聽聽聽聽> What's that ?
聽聽聽聽>
聽聽聽聽> As soon as a contact is created[subscribed] .. the user must provide
聽聽聽聽> server name/ip address of the remote machine before its added to
聽聽聽聽> contacts list ...
聽聽聽聽>
聽聽聽聽> The above makes the config form of remote machine visible in the
聽聽聽聽> subsribe method..

聽聽聽聽Isn't this the role of the account creation wizzard ?

No. Each contact needs to have its own info( server, account etc)

聽聽聽聽>
聽聽聽聽> * createUnresolvedContact : startTimerTask is perhaps not
聽聽聽聽needed as,
聽聽聽聽> normally, an unresolved contact doesn't generate network
聽聽聽聽> communications.
聽聽聽聽>
聽聽聽聽> Contacts can only be resolved the Timer Task [update its status in
聽聽聽聽> Contact list]
聽聽聽聽> anyways timer task just checks for reachability [just an innocent
聽聽聽聽> open/close on ssh port presently]

聽聽聽聽I don't know of what to do of this but to me it seems that having a
聽聽聽聽network connection in this method is not valid considering the
聽聽聽聽javadoc.

we need to have some network communication to get the status of the machine.
As Java 1.5 is in, I am using InetAddress.isReachable() method now.
the javadoc stands corrected.

聽聽聽聽>
聽聽聽聽> * use protocolNames for the name of the service in the Osgi
聽聽聽聽requests
聽聽聽聽>
聽聽聽聽> I didnt get the above .. same for all protocolNames.SSH below

聽聽聽聽There is a class nammed protocolNames which role is to contain any
聽聽聽聽protocol name. It's better to use this than using dirty String.
聽聽聽聽You can
聽聽聽聽add a value for the SSH protocol in it and use it.

Agreed! I wasn't aware of this earlier

聽聽聽聽>
聽聽聽聽> * why unresolvedContact have resolved=true and volatile
聽聽聽聽> resolved=false ?
聽聽聽聽>
聽聽聽聽> there are no volatile contacts in SSH .. this code is redundant
聽聽聽聽> [an unknown/unspecified machine can't send a messages to a user]

聽聽聽聽I just wanted to point the incoherence here as SSH unresolved
聽聽聽聽contacts
聽聽聽聽have resolved = true while they are by definition unresolved and
聽聽聽聽volatile ones have resolved=false, to me, it looks a little bit
聽聽聽聽strange.

Agreed, it does seem strange.

My understanding is that SSH contacts don't appear on their own like volatile contacts (so volatile resolved is set to false)
and as user manually adds each contact, they are automatically resolved.

Please correct me.

聽聽聽聽>
聽聽聽聽> ppFactorySSHImpl
聽聽聽聽>
聽聽聽聽> * use ProtocolNames.SSH here again
聽聽聽聽>

Corrected!

聽聽聽聽> didn't get this
聽聽聽聽>
聽聽聽聽> ppFactorySSH
聽聽聽聽> * again, this class is a clever solution !
聽聽聽聽>
聽聽聽聽> ppServiceSSHImpl
聽聽聽聽> * use ProtocolNames.SSH here again
聽聽聽聽>

Corrected!

聽聽聽聽> didn't get this
聽聽聽聽>
聽聽聽聽> Resources (both)
聽聽聽聽>
聽聽聽聽> * getImage : the array has a fixed size, it's dirty and may
聽聽聽聽cause some
聽聽聽聽> problem, why not use the InputStream.available to fix a dynamic
聽聽聽聽> size for
聽聽聽聽> the byte array ?
聽聽聽聽>
聽聽聽聽> Corrected
聽聽聽聽>
聽聽聽聽> resources.properties
聽聽聽聽> * Some parameters haven't their place here like the timeout
聽聽聽聽value. To
聽聽聽聽> see where to put these parameters, take a look at what Yana
聽聽聽聽do to save
聽聽聽聽> the last status of an account.
聽聽聽聽>
聽聽聽聽What about this ?

Extra parameters have been removed.

聽聽聽聽>
聽聽聽聽> * the testCommand and response aren't used, why keep it on
聽聽聽聽this file ?
聽聽聽聽>
聽聽聽聽> These are temporary solutions to tricky problems .. a better soln
聽聽聽聽> needs to be found.

聽聽聽聽If they are unused, why keeping them stored in a file ?

Extra parameters have been removed.

聽聽聽聽>
聽聽聽聽> SSHActivator
聽聽聽聽>
聽聽聽聽> * use ProtocolNames.SSH here again
聽聽聽聽>
聽聽聽聽> didn't get this
聽聽聽聽>
聽聽聽聽> SSHUserInfo
聽聽聽聽>
聽聽聽聽> * promptYesNo : The dialog type is "warning" however you're
聽聽聽聽asking a
聽聽聽聽> question so why didn't you chose the "question" type ?
聽聽聽聽>
聽聽聽聽> Corrected
聽聽聽聽>
聽聽聽聽> resources.properties in the wizzard
聽聽聽聽>
聽聽聽聽> * use ProtocolNames.SSH here again for the protocol name
聽聽聽聽>
聽聽聽聽> didn't get this
聽聽聽聽>
聽聽聽聽> SSHAccountRegistration
聽聽聽聽>
聽聽聽聽> * identityFile : I dont understand what it is. Can you tell
聽聽聽聽it in the
聽聽聽聽> javadoc ?
聽聽聽聽>
聽聽聽聽> Identity file is a private[default] key of the user which is one of
聽聽聽聽> the methods of authentication [its a standard in SSH]

聽聽聽聽Please add this in javadoc :slight_smile:

Added :slight_smile:

聽聽聽聽>
聽聽聽聽> * Why the server port is a String and not an int in the
聽聽聽聽methods ?
聽聽聽聽>
聽聽聽聽> same as in OpSetContactInfo
聽聽聽聽>
聽聽聽聽> And that's all.
聽聽聽聽> Once again, don't worry about this list, you've done a great
聽聽聽聽job
聽聽聽聽> Jindal !
聽聽聽聽> However if you want your baby live in SC, you'll need to
聽聽聽聽provide us a
聽聽聽聽> little patch which correct all this and especially the
聽聽聽聽little security
聽聽聽聽> problem.
聽聽聽聽>
聽聽聽聽> Cheers,
聽聽聽聽> Ben
聽聽聽聽>
聽聽聽聽> ---------------------------------------------------------------------
聽聽聽聽> To unsubscribe, e-mail:
聽聽聽聽> dev-unsubscribe@sip-communicator.dev.java.net
聽聽聽聽<mailto:dev-unsubscribe@sip-communicator.dev.java.net>
聽聽聽聽> <mailto:dev-unsubscribe@sip-communicator.dev.java.net
聽聽聽聽<mailto:dev-unsubscribe@sip-communicator.dev.java.net>>
聽聽聽聽> For additional commands, e-mail:
聽聽聽聽> dev-help@sip-communicator.dev.java.net
聽聽聽聽<mailto:dev-help@sip-communicator.dev.java.net>
聽聽聽聽> <mailto: dev-help@sip-communicator.dev.java.net
聽聽聽聽<mailto:dev-help@sip-communicator.dev.java.net>>
聽聽聽聽>
聽聽聽聽> --
聽聽聽聽> Shobhit Jindal
聽聽聽聽>
聽聽聽聽------------------------------------------------------------------------
聽聽聽聽>
聽聽聽聽---------------------------------------------------------------------
聽聽聽聽> To unsubscribe, e-mail:
聽聽聽聽dev-unsubscribe@sip-communicator.dev.java.net
聽聽聽聽<mailto:dev-unsubscribe@sip-communicator.dev.java.net>
聽聽聽聽> For additional commands, e-mail:
聽聽聽聽dev-help@sip-communicator.dev.java.net
聽聽聽聽<mailto:dev-help@sip-communicator.dev.java.net>

聽聽聽聽Here we are. Thanks for your first corrections, I'll wait your next
聽聽聽聽patch to commit all these correction in one patch.

聽聽聽聽Cheers,
聽聽聽聽Ben.

聽聽聽聽---------------------------------------------------------------------
聽聽聽聽To unsubscribe, e-mail:
聽聽聽聽dev-unsubscribe@sip-communicator.dev.java.net
聽聽聽聽<mailto:dev-unsubscribe@sip-communicator.dev.java.net>
聽聽聽聽For additional commands, e-mail:
聽聽聽聽dev-help@sip-communicator.dev.java.net
聽聽聽聽<mailto:dev-help@sip-communicator.dev.java.net>

--
Shobhit Jindal
B.Tech. Part-IV,
Department Of Electronics Engineering,
Centre of Advanced Studies,
Institute Of Technology, BHU
------------------------------------------------------------------------

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#10

Hi Benoit,

Please let me know when you import.

路路路

On Jan 7, 2008 10:02 PM, Benoit Pradelle <ze_real_neo@yahoo.fr> wrote:

Hi Shobhit,

Thanks for your answer, I'm a little bit busy these days but I'll import
your modification and activate SSH in SC as soon as possible.

Cheers,
Ben


#11

waiting for the import ....

路路路

On Jan 11, 2008 3:09 PM, Shobhit Jindal <shobhit.jindal.ece04@itbhu.ac.in> wrote:

Hi Benoit,

Please let me know when you import.

On Jan 7, 2008 10:02 PM, Benoit Pradelle <ze_real_neo@yahoo.fr> wrote:

> Hi Shobhit,
>
> Thanks for your answer, I'm a little bit busy these days but I'll import
> your modification and activate SSH in SC as soon as possible.
>
> Cheers,
> Ben
>

--
Shobhit Jindal
B.Tech. Part-IV,
Department Of Electronics Engineering,
Centre of Advanced Studies,
Institute Of Technology, BHU


#12

Hi Shobhit,

Don't worry, I don't forget you, i'm just very busy these days. I'll do it soon now and you'll be informed when done.

Cheers,
Ben

Shobhit Jindal a 茅crit :

路路路

waiting for the import ....

On Jan 11, 2008 3:09 PM, Shobhit Jindal > <shobhit.jindal.ece04@itbhu.ac.in > <mailto:shobhit.jindal.ece04@itbhu.ac.in>> wrote:

聽聽聽聽Hi Benoit,

聽聽聽聽Please let me know when you import.

聽聽聽聽On Jan 7, 2008 10:02 PM, Benoit Pradelle < ze_real_neo@yahoo.fr > <mailto:ze_real_neo@yahoo.fr>> wrote:

聽聽聽聽聽聽聽聽Hi Shobhit,

聽聽聽聽聽聽聽聽Thanks for your answer, I'm a little bit busy these days but
聽聽聽聽聽聽聽聽I'll import
聽聽聽聽聽聽聽聽your modification and activate SSH in SC as soon as possible.

聽聽聽聽聽聽聽聽Cheers,
聽聽聽聽聽聽聽聽Ben

--
Shobhit Jindal
B.Tech. Part-IV,
Department Of Electronics Engineering,
Centre of Advanced Studies,
Institute Of Technology, BHU

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#13

Hi Shobhit, all,

Finally we get it ! Thanks to the latest patch made by Shobhit, SSH is now fixed and activated in SIP Communicator. Thanks for your work Shobhit, your latest patch was very good !

Cheers,
Ben

路路路

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#14

Hi all,

I couldn't connect sip-communicator to yahoo. The problem as follow
Connection failed for the following account...
Server name: yahoo.com

What's the problem? How could I change yahoo server in sip-comm? It is neccessary to modify code?

Thanks in advance,
Julien Nguyen.

路路路

---------------------------------
Ne gardez plus qu'une seule adresse mail ! Copiez vos mails vers Yahoo! Mail


#15

Hi Ben,

Yahoo! my patch is in :slight_smile: Thanks a lot! will look forward to improve it.

Am already thinking abt next GSoC. Its abt encrypted encryptions voice
and/or chat and be compatible with other clients who offer such
facilities(like pidigin), or mebbe like that for Jabber [RFC3923]. Am not
aware if any standards exist for voice chat.

are any such efforts already underway?

路路路

-
Shobhit

On Feb 4, 2008 10:22 PM, Benoit Pradelle <ze_real_neo@yahoo.fr> wrote:

Hi Shobhit, all,

Finally we get it ! Thanks to the latest patch made by Shobhit, SSH is
now fixed and activated in SIP Communicator. Thanks for your work
Shobhit, your latest patch was very good !

Cheers,
Ben

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#16

Hi Benoit and Shobhit,

The SSH bundle is very fun and can be very useful.
Just a little remark, the shell display does not seems to manage UTF-8 (see image attached). A second remark is that the text is replaced by smiley, can you disable the smiley substitution process ?

However, a great work !

Vincent

Benoit Pradelle wrote:

路路路

Hi Shobhit, all,

Finally we get it ! Thanks to the latest patch made by Shobhit, SSH is now fixed and activated in SIP Communicator. Thanks for your work Shobhit, your latest patch was very good !

Cheers,
Ben

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#17

Hi,

are you using any proxy for connecting to internet ?
the server is in the code actually and cannot be changed for now.
Can you connect any other protocol ?
Cause we are using yahoo and we are running tests and there are no errors.

damencho

Minh Nguyen wrote:

路路路

Hi all,

I couldn't connect sip-communicator to yahoo. The problem as follow
Connection failed for the following account...
Server name: yahoo.com

What's the problem? How could I change yahoo server in sip-comm? It is neccessary to modify code?

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#18

Hi Shobhit,

Congrats for your great work on SSH!

路路路

On 2008/02/05, at 9:52, Shobhit Jindal wrote:

Am already thinking abt next GSoC. Its abt encrypted encryptions voice and/or chat and be compatible with other clients who offer such facilities(like pidigin), or mebbe like that for Jabber [RFC3923]. Am not aware if any standards exist for voice chat.

Actually we have an implementation of SRTP from Su Bing (from last GSoC too), but it still lacks the key exchange mechanism, which will most probably be part of the next GSoC if we are accepted to the program this year.

Cheers,
romain

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#19

I had problems with yahoo today but they all disappeared in a couple of
hours. You might want to try connecting again at some point as your
problems could be caused by some temporary issues with the servers.

Emil

Damian Minkov 薪邪锌懈褋邪:

路路路

Hi,

are you using any proxy for connecting to internet ?
the server is in the code actually and cannot be changed for now.
Can you connect any other protocol ?
Cause we are using yahoo and we are running tests and there are no errors.

damencho

Minh Nguyen wrote:

Hi all,

I couldn't connect sip-communicator to yahoo. The problem as follow
Connection failed for the following account...
Server name: yahoo.com

What's the problem? How could I change yahoo server in sip-comm? It is
neccessary to modify code?

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#20

I still have problems with yahoo, I connected many time with never success. The connection is directed and doesn't have any proxy.

I do appreciate for your any suggestions.

Emil Ivov <emcho@sip-communicator.org> a 锟絚rit : I had problems with yahoo today but they all disappeared in a couple of
hours. You might want to try connecting again at some point as your
problems could be caused by some temporary issues with the servers.

Emil

Damian Minkov 薪邪锌懈褋邪:

路路路

Hi,

are you using any proxy for connecting to internet ?
the server is in the code actually and cannot be changed for now.
Can you connect any other protocol ?
Cause we are using yahoo and we are running tests and there are no errors.

damencho

Minh Nguyen wrote:

Hi all,

I couldn't connect sip-communicator to yahoo. The problem as follow
Connection failed for the following account...
Server name: yahoo.com

What's the problem? How could I change yahoo server in sip-comm? It is
neccessary to modify code?

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------
Ne gardez plus qu'une seule adresse mail ! Copiez vos mails vers Yahoo! Mail