[PATCH 4/6] s3-spoolss: Hookup port creation basing name on parsed CUPS URIs

[David Disseldorp]

On Wed, 31 Aug 2011 15:03:12 -0400
Justin Chevrier <jchevrier at gmail.com> wrote:

...
> +static bool get_printer_port(TALLOC_CTX *mem_ctx,
> +				 const char *printername,
> +				 const char **portname,
> +				 const char **ipaddress)
> +{
> +	const char *uri = NULL;
> +	char *p, *tmpuri;
> +	NTSTATUS nt_status;
> +
> +	nt_status = printer_list_get_printer(mem_ctx,
> +					     printername,
> +					     NULL,
> +					     NULL,
> +					     &uri,
> +					     NULL);
> +
> +	if (NT_STATUS_IS_OK(nt_status)) {
> +		if (uri != NULL) {
> +			/* We only care about network URIs */
> +			if (uri == strstr(uri, "ipp://") || uri == strstr(uri, "http://") ||
> +			    uri == strstr(uri, "lpd://") || uri == strstr(uri, "https://") ||
> +			    uri == strstr(uri, "socket://")) {

I'd prefer to see this reworked into something a little easier on the
eye like:
        if (!NT_STATUS_IS_OK(nt_status)) {
                goto done;
        } else if ((uri != NULL) && (uri_is_network(uri))) {


> +			    	uri = strchr(uri, ':');
> +				uri += 3;
> +
> +				p = talloc_strdup(mem_ctx, uri);
> +				if (p == NULL) {
> +					return false;
> +				}
> +
> +				if ((tmpuri = strchr(p, ':')) != NULL) {
> +					*tmpuri = '\0';
> +				}
> +				if ((tmpuri = strchr(p, '/')) != NULL) {
> +					*tmpuri = '\0';
> +				}
> +
> +				/* If what's left after stripping extra characters isn't a valid
> +				 * IP address, give up. Should we do a DNS lookup on the string? */

Yes, I expect hostnames as well as IPv4 & IPv6 addresses should be
handled here.

...
> +					*ipaddress = talloc_strdup(mem_ctx, p);
> +					if(*ipaddress == NULL) {
> +						return false;
> +					}

A space is missing between the if and opening bracket here, also there
are a couple of talloc_free's missing from the error paths.