Date: Mon, 29 Oct 2012 21:15:22 -0700
From: Dmitry Torokhov <dtor@vmware.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: George Zhang <georgezhang@vmware.com>, pv-drivers@vmware.com,
        linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org
Subject: Re: [Pv-drivers] [PATCH 04/12] VMCI: device driver implementaton.
Message-ID: <20121030041522.GC32055@dtor-ws.eng.vmware.com>
References: <20121030005923.17788.21797.stgit@promb-2n-dhcp175.eng.vmware.com>
 <20121030010409.17788.6745.stgit@promb-2n-dhcp175.eng.vmware.com>
 <20121030022347.GE1920@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20121030022347.GE1920@kroah.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3152
Lines: 123

On Mon, Oct 29, 2012 at 07:23:47PM -0700, Greg KH wrote:
> On Mon, Oct 29, 2012 at 06:04:15PM -0700, George Zhang wrote:
> > +static int __init vmci_core_init(void)
> > +{
> > +	int result;
> > +
> > +	result = vmci_ctx_init();
> > +	if (result < VMCI_SUCCESS) {
> > +		pr_err("Failed to initialize VMCIContext (result=%d).\n",
> > +			result);
> 
> If you are going to use pr_* functions, it's usually a good idea to
> define pr_fmt in a consistant way so that it shows up the same across
> all of your .c files.  See other drivers for examples of how to do this
> properly.

pr_fmt() is defined in drivers/misc/vmw_vmci/vmci_common_int.h

> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	result = vmci_datagram_init();
> > +	if (result < VMCI_SUCCESS) {
> > +		pr_err("Failed to initialize VMCIDatagram (result=%d).\n",
> > +			result);
> > +		return -EINVAL;
> > +	}
> > +
> > +	result = vmci_event_init();
> > +	if (result < VMCI_SUCCESS) {
> > +		pr_err("Failed to initialize VMCIEvent (result=%d).\n", result);
> > +		return -EINVAL;
> > +	}
> 
> You don't free and unwind things properly if things go wrong here, why
> not?

We do, the above calls do not need to be cleaned up.
 
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void __exit vmci_core_exit(void)
> > +{
> > +	vmci_event_exit();
> > +}
> > +
> > +static int __init vmci_drv_init(void)
> > +{
> > +	int error;
> > +
> > +	error = vmci_core_init();
> > +	if (error)
> > +		return error;
> > +
> > +	if (!vmci_disable_guest) {
> > +		error = vmci_guest_init();
> > +		if (error) {
> > +			pr_warn("Failed to initialize guest personality (err=%d).\n",
> > +				error);
> > +		} else {
> > +			vmci_guest_personality_initialized = true;
> > +			pr_info("Guest personality initialized and is %s\n",
> > +				vmci_guest_code_active() ?
> > +				"active" : "inactive");
> > +		}
> > +	}
> > +
> > +	if (!vmci_disable_host) {
> > +		error = vmci_host_init();
> > +		if (error) {
> > +			pr_warn("Unable to initialize host personality (err=%d).\n",
> > +				error);
> > +		} else {
> > +			vmci_host_personality_initialized = true;
> > +			pr_info("Initialized host personality\n");
> > +		}
> > +	}
> > +
> > +	if (!vmci_guest_personality_initialized &&
> > +	    !vmci_host_personality_initialized) {
> > +		vmci_core_exit();
> > +		return -ENODEV;
> > +	}
> > +
> > +	pr_debug("Module is initialized\n");
> 
> Really?  You need this message still?

These are debug messages so do not show up in logs by default.

> 
> > +	return 0;
> > +}
> > +module_init(vmci_drv_init);
> > +
> > +static void __exit vmci_drv_exit(void)
> > +{
> > +	if (vmci_guest_personality_initialized)
> > +		vmci_guest_exit();
> > +
> > +	if (vmci_host_personality_initialized)
> > +		vmci_host_exit();
> > +
> > +	vmci_core_exit();
> > +	pr_debug("Module is unloaded\n");
> 
> Same here, is this really needed?
> 

Still debug so no littering in logs.

Thanks,
Dmitry

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/