[libvirt] [PATCH] Explicitly error on uri=qemu://system

[Cole Robinson]

On 04/19/2016 04:30 AM, Peter Krempa wrote:
> On Mon, Apr 18, 2016 at 19:04:04 -0400, Cole Robinson wrote:
>> It's a fairly common error that a user tries to connect to a URI
>> like qemu://system or qemu://session (missing a slash). This errors
>> like:
>>
>> $ virsh --connect qemu://session
>> error: failed to connect to the hypervisor
>> error: Unable to resolve address 'session' service '16514': No address associated with hostname
>>
>> If you already know that the standard qemu URI has 3 slashes, that
>> error will make it obvious enough. But new user's may not get it.
>> There's even a RHEL support page explicitly mentioning it!:
>>
>> https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Virtualization_Deployment_and_Administration_Guide/sect-Troubleshooting-Common_libvirt_errors_and_troubleshooting.html
>>
>> Catch this error early in libvirt.c virConnectOpen for qemu (and vbox
>> which has similar rules
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1038304
>> ---
>>  src/libvirt.c | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/src/libvirt.c b/src/libvirt.c
>> index a21d00e..7607ae3 100644
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>> @@ -928,6 +928,33 @@ virConnectGetDefaultURI(virConfPtr conf,
>>  }
>>  
>>  
>> +/*
>> + * Check to see if an invalid URI like qemu://system (missing /) was passed,
>> + * offer the suggested fix.
>> + */
>> +static int
>> +check_uri_missing_slash(const char *uristr, virURIPtr uri)
> 
> Please use a name with "vir" prefix and camel case. This is totaly
> against our naming convention.
> 

Yes I know, but I was just following the pattern of the do_open function
below. I'll change it to virConnectCheckURIMissingSlash

>> +{
>> +    /* These drivers _only_ accepts URIs with a 'path' element */
> 
> Only these drivers accept ... ? I don't quite follow the message of this
> comment.
> 

I'll change it to

/* To avoid false positives, only check drivers that mandate
   a path component in the URI, like /system or /session */

>> +    if (STRNEQ(uri->sceme, "qemu") &&
>> +        STRNEQ(uri->scheme, "vbox"))
>> +        return 0;
>> +
>> +    if (uri->path != NULL)
>> +        return 0;
>> +
>> +    if (STREQ(uri->server, "session") ||
>> +        STREQ(uri->server, "system")) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("invalid URI %s (maybe you want %s:///%s)"),
>> +                       uristr, uri->scheme, uri->server);
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
> 
> ACK with the rename and fix of the comment.
> 

Thanks, I'll send a v2 though with the vz change

- Cole