[libvirt] [PATCH v2 2/2] libssh_transport: add new libssh-based transport

[Pino Toscano]

On Wednesday, 19 October 2016 08:33:27 CEST Peter Krempa wrote:
> > > > +    memset(&retr_passphrase, 0, sizeof(virConnectCredential));
> > > > +    retr_passphrase.type = virCredTypeForPrompt(sess->cred, echo);
> > > > +    retr_passphrase.prompt = virBufferCurrentContent(&buff);
> > > 
> > > This shouldn't really be used. Please get the content into a variable.
> > 
> > Not a problem to change this, but could you please explain a bit more
> > on the reasons? (So I can avoid doing the same again.)
> 
> Basically any call any function that uses the buffer invalidates the
> pointer returned by virBufferCurrentContent (or should be treated as
> such) which is prone to bugs when later modifying the code.
> 
> Most of the uses of the function are in special cases (or should be
> removed).

I see, thanks for the explanation.

> > > > +int
> > > > +virNetLibsshSessionConnect(virNetLibsshSessionPtr sess,
> > > > +                           int sock)
> > > > +{
> > > > +    int ret;
> > > > +    const char *errmsg;
> > > > +
> > > > +    VIR_DEBUG("sess=%p, sock=%d", sess, sock);
> > > > +
> > > > +    if (!sess || sess->state != VIR_NET_LIBSSH_STATE_NEW) {
> > > > +        virReportError(VIR_ERR_LIBSSH, "%s",
> > > > +                       _("Invalid virNetLibsshSessionPtr"));
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    virObjectLock(sess);
> > > > +
> > > > +    /* check if configuration is valid */
> > > > +    if ((ret = virNetLibsshValidateConfig(sess)) < 0)
> > > > +        goto error;
> > > > +
> > > > +    /* read ~/.ssh/config */
> > > > +    if ((ret = ssh_options_parse_config(sess->session, NULL)) < 0)
> > > > +        goto error;
> > > > +
> > > > +    /* set the socket FD for the libssh session */
> > > > +    if ((ret = ssh_options_set(sess->session, SSH_OPTIONS_FD, &sock)) < 0)
> > > 
> > > Is this guaranteed to copy the socket number at call time? Otherwise
> > > (similarly to the ones above will not work reliably).
> > 
> > I don't understand this sentence (it seems truncated), can you please
> > rephrase it?
> 
> Sorry I probably got sidetracked and did not finish my thought.
> 
> My question was whether ssh_options_set accesses the pointer to 'sock'
> right away and copies it. You are passing a pointer to a stack allocated
> variable which will get out of scope later, thus the pointer should not
> be accessed after the call to ssh_options_set returns.

libssh indeed copies the values passed to ssh_options_set, so the two
calls you mentioned (for SSH_OPTIONS_FD and SSH_OPTIONS_PORT) are safe.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161019/35ea3a3b/attachment-0001.sig>