[libvirt] [PATCH v3 02/11] virt-admin: Introduce first working skeleton

[Erik Skultety]

>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library.  If not, see
>> + * <http://www.gnu.org/licenses/>.
>> + *
>> + * Erik Skultety <eskultet at redhat.com>
> 
> We usually prepend this line with "Authors: ".
> 

How can I keep forgetting these things?! I'll fix it, thanks.

>> +static bool
>> +cmdConnect(vshControl *ctl, const vshCmd *cmd)
>> +{
>> +    const char *name = NULL;
>> +    vshAdmControlPtr priv = ctl->privData;
>> +
>> +    VIR_FREE(ctl->connname);
>> +    if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0)
>> +        return false;
> 
> I think the VIR_FREE() should go after option acquiring. Otherwise we
> will lose it if vshCommand... fails.
> 

Spurious as it might be, this could only fail, if we decided that --name
argument to virsh/virt-admin connect command should be required, but
since it's optional, vshCommandOptStringReq can never fail in this
specific case (I guess I already mentioned this in one of my earlier
replies to previous versions). But I do agree with you that it would be
nice to fix the logic, so no future questions about this snippet of code
will arise.

>> +    ctl->connname = vshStrdup(ctl, name);
>> +
>> +    vshAdmReconnect(ctl);
>> +
>> +    return !!priv->conn;
>> +}
> 
> ACK
> 
> Michal
> 

Thanks, I'll adjust the patch and push it once the series is complete.

Erik