[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3] xen-scsiback: define a pr_fmt macro with xen-pvscsi





On 2015/3/6 20:53, Juergen Gross wrote:
On 03/06/2015 09:34 PM, Tao Chen wrote:
Add the {xen-pvscsi: } prefix in pr_fmt and remove DPRINTK, then
replace all DPRINTK with pr_debug.

Also fixed up some comments just as eliminate redundant whitespace
and format the code.

These will make the code easier to read.

Signed-off-by: Tao Chen <boby.chen@xxxxxxxxxx>
---
  drivers/xen/xen-scsiback.c | 72
++++++++++++++++++++++++----------------------
  1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 9faca6a..919397e 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -69,8 +69,10 @@
  #include <xen/interface/grant_table.h>
  #include <xen/interface/io/vscsiif.h>

-#define DPRINTK(_f, _a...)            \
-    pr_debug("(file=%s, line=%d) " _f, __FILE__ , __LINE__ , ## _a)
+#ifdef pr_fmt
+#undef pr_fmt
+#endif
+#define pr_fmt(fmt) "xen-pvscsi: " fmt

Have you looked around in the kernel how pr_fmt() is used normally?
You can spare 3 lines by placing the #define above all #include lines.


Many thanks for pointing out my mistake. I will correct it.


  #define VSCSI_VERSION    "v0.1"
  #define VSCSI_NAMELEN    32
@@ -271,7 +273,7 @@ static void scsiback_print_status(char
*sense_buffer, int errors,
  {
      struct scsiback_tpg *tpg = pending_req->v2p->tpg;

-    pr_err("xen-pvscsi[%s:%d] cmnd[0]=%02x -> st=%02x msg=%02x
host=%02x drv=%02x\n",
+    pr_err("[%s:%d] cmnd[0]=%02x -> st=%02x msg=%02x host=%02x
drv=%02x\n",
             tpg->tport->tport_name, pending_req->v2p->lun,
             pending_req->cmnd[0], status_byte(errors), msg_byte(errors),
             host_byte(errors), driver_byte(errors));
@@ -427,7 +429,7 @@ static int scsiback_gnttab_data_map_batch(struct
gnttab_map_grant_ref *map,
      BUG_ON(err);
      for (i = 0; i < cnt; i++) {
          if (unlikely(map[i].status != GNTST_okay)) {
-            pr_err("xen-pvscsi: invalid buffer -- could not remap
it\n");
+            pr_err("invalid buffer -- could not remap it\n");
              map[i].handle = SCSIBACK_INVALID_HANDLE;
              err = -ENOMEM;
          } else {
@@ -449,7 +451,7 @@ static int scsiback_gnttab_data_map_list(struct
vscsibk_pend *pending_req,
      for (i = 0; i < cnt; i++) {
          if (get_free_page(pg + mapcount)) {
              put_free_pages(pg, mapcount);
-            pr_err("xen-pvscsi: no grant page\n");
+            pr_err("no grant page\n");
              return -ENOMEM;
          }
          gnttab_set_map_op(&map[mapcount], vaddr_page(pg[mapcount]),
@@ -492,7 +494,7 @@ static int scsiback_gnttab_data_map(struct
vscsiif_request *ring_req,
          return 0;

      if (nr_segments > VSCSIIF_SG_TABLESIZE) {
-        DPRINTK("xen-pvscsi: invalid parameter nr_seg = %d\n",
+        pr_debug("invalid parameter nr_seg = %d\n",
              ring_req->nr_segments);
          return -EINVAL;
      }
@@ -516,13 +518,13 @@ static int scsiback_gnttab_data_map(struct
vscsiif_request *ring_req,
              nr_segments += n_segs;
          }
          if (nr_segments > SG_ALL) {
-            DPRINTK("xen-pvscsi: invalid nr_seg = %d\n",
+            pr_debug("invalid nr_seg = %d\n",
                  nr_segments);

Above 2 lines fit into a single one now.


I will correct it.

              return -EINVAL;
          }
      }

-    /* free of (sgl) in fast_flush_area()*/
+    /* free of (sgl) in fast_flush_area() */
      pending_req->sgl = kmalloc_array(nr_segments,
                      sizeof(struct scatterlist), GFP_KERNEL);
      if (!pending_req->sgl)
@@ -679,7 +681,7 @@ static int prepare_pending_reqs(struct
vscsibk_info *info,
      v2p = scsiback_do_translation(info, &vir);
      if (!v2p) {
          pending_req->v2p = NULL;
-        DPRINTK("xen-pvscsi: doesn't exist.\n");
+        pr_debug("doesn't exist.\n");

Without the line information this message is pretty meaningless.

For this suggestion, I want to modify this print like below

pr_debug("the v2p of (chn:%d, tgt:%d, lun:%d) doesn't exist.\n",
                 vir.chn, vir.tgt, vir.lun);

What do you think, Juergen?


          return -ENODEV;
      }
      pending_req->v2p = v2p;
@@ -690,14 +692,14 @@ static int prepare_pending_reqs(struct
vscsibk_info *info,
          (pending_req->sc_data_direction != DMA_TO_DEVICE) &&
          (pending_req->sc_data_direction != DMA_FROM_DEVICE) &&
          (pending_req->sc_data_direction != DMA_NONE)) {
-        DPRINTK("xen-pvscsi: invalid parameter data_dir = %d\n",
+        pr_debug("invalid parameter data_dir = %d\n",
              pending_req->sc_data_direction);
          return -EINVAL;
      }

      pending_req->cmd_len = ring_req->cmd_len;
      if (pending_req->cmd_len > VSCSIIF_MAX_COMMAND_SIZE) {
-        DPRINTK("xen-pvscsi: invalid parameter cmd_len = %d\n",
+        pr_debug("invalid parameter cmd_len = %d\n",
              pending_req->cmd_len);
          return -EINVAL;
      }
@@ -721,7 +723,7 @@ static int scsiback_do_cmd_fn(struct vscsibk_info
*info)

      if (RING_REQUEST_PROD_OVERFLOW(ring, rp)) {
          rc = ring->rsp_prod_pvt;
-        pr_warn("xen-pvscsi: Dom%d provided bogus ring requests (%#x
- %#x = %u). Halting ring processing\n",
+        pr_warn("Dom%d provided bogus ring requests (%#x - %#x = %u).
Halting ring processing\n",
                 info->domid, rp, rc, rp - rc);
          info->ring_error = 1;
          return 0;
@@ -772,7 +774,7 @@ static int scsiback_do_cmd_fn(struct vscsibk_info
*info)
              scsiback_device_action(pending_req, TMR_LUN_RESET, 0);
              break;
          default:
-            pr_err_ratelimited("xen-pvscsi: invalid request\n");
+            pr_err_ratelimited("invalid request\n");
              scsiback_do_resp_with_sense(NULL, DRIVER_ERROR << 24,
                              0, pending_req);
              kmem_cache_free(scsiback_cachep, pending_req);
@@ -874,14 +876,14 @@ static int scsiback_add_translation_entry(struct
vscsibk_info *info,

      lunp = strrchr(phy, ':');
      if (!lunp) {
-        pr_err("xen-pvscsi: illegal format of physical device %s\n",
+        pr_err("illegal format of physical device %s\n",
              phy);

Merge lines.

I will correct it.


          return -EINVAL;
      }
      *lunp = 0;
      lunp++;
      if (kstrtouint(lunp, 10, &lun) || lun >=
TRANSPORT_MAX_LUNS_PER_TPG) {
-        pr_err("xen-pvscsi: lun number not valid: %s\n", lunp);
+        pr_err("lun number not valid: %s\n", lunp);
          return -EINVAL;
      }

@@ -909,7 +911,7 @@ static int scsiback_add_translation_entry(struct
vscsibk_info *info,
      mutex_unlock(&scsiback_mutex);

      if (!tpg) {
-        pr_err("xen-pvscsi: %s:%d %s\n", phy, lun, error);
+        pr_err("%s:%d %s\n", phy, lun, error);
          return -ENODEV;
      }

@@ -926,7 +928,7 @@ static int scsiback_add_translation_entry(struct
vscsibk_info *info,
          if ((entry->v.chn == v->chn) &&
              (entry->v.tgt == v->tgt) &&
              (entry->v.lun == v->lun)) {
-            pr_warn("xen-pvscsi: Virtual ID is already used.
Assignment was not performed.\n");
+            pr_warn("Virtual ID is already used. Assignment was not
performed.\n");
              err = -EEXIST;
              goto out;
          }
@@ -997,7 +999,7 @@ static void scsiback_do_add_lun(struct
vscsibk_info *info, const char *state,
      if (!scsiback_add_translation_entry(info, phy, vir)) {
          if (xenbus_printf(XBT_NIL, info->dev->nodename, state,
                    "%d", XenbusStateInitialised)) {
-            pr_err("xen-pvscsi: xenbus_printf error %s\n", state);
+            pr_err("xenbus_printf error %s\n", state);
              scsiback_del_translation_entry(info, vir);
          }
      } else {
@@ -1012,7 +1014,7 @@ static void scsiback_do_del_lun(struct
vscsibk_info *info, const char *state,
      if (!scsiback_del_translation_entry(info, vir)) {
          if (xenbus_printf(XBT_NIL, info->dev->nodename, state,
                    "%d", XenbusStateClosed))
-            pr_err("xen-pvscsi: xenbus_printf error %s\n", state);
+            pr_err("xenbus_printf error %s\n", state);
      }
  }

@@ -1071,7 +1073,7 @@ static void scsiback_do_1lun_hotplug(struct
vscsibk_info *info, int op,
              /* modify vscsi-devs/dev-x/state */
              if (xenbus_printf(XBT_NIL, dev->nodename, state,
                        "%d", XenbusStateConnected)) {
-                pr_err("xen-pvscsi: xenbus_printf error %s\n",
+                pr_err("xenbus_printf error %s\n",
                         str);

Merge lines.

I will correct it.


                  scsiback_del_translation_entry(info, &vir);
                  xenbus_printf(XBT_NIL, dev->nodename, state,
@@ -1079,7 +1081,7 @@ static void scsiback_do_1lun_hotplug(struct
vscsibk_info *info, int op,
              }
          }
          break;
-    /*When it is necessary, processing is added here.*/
+    /* When it is necessary, processing is added here. */
      default:
          break;
      }
@@ -1196,7 +1198,7 @@ static int scsiback_probe(struct xenbus_device
*dev,
      struct vscsibk_info *info = kzalloc(sizeof(struct vscsibk_info),
                          GFP_KERNEL);

-    DPRINTK("%p %d\n", dev, dev->otherend_id);
+    pr_debug("%p %d\n", dev, dev->otherend_id);

Meaningless without more info.


For this suggestion, I want to modify this print like below

pr_debug("%s %p %d\n", __func__, dev, dev->otherend_id);

What do you think, Juergen?


Juergen


      if (!info) {
          xenbus_dev_fatal(dev, -ENOMEM, "allocating backend structure");
@@ -1227,7 +1229,7 @@ static int scsiback_probe(struct xenbus_device
*dev,
      return 0;

  fail:
-    pr_warn("xen-pvscsi: %s failed\n", __func__);
+    pr_warn("%s failed\n", __func__);
      scsiback_remove(dev);

      return err;
@@ -1432,7 +1434,7 @@ check_len:
      }
      snprintf(&tport->tport_name[0], VSCSI_NAMELEN, "%s", &name[off]);

-    pr_debug("xen-pvscsi: Allocated emulated Target %s Address: %s\n",
+    pr_debug("Allocated emulated Target %s Address: %s\n",
           scsiback_dump_proto_id(tport), name);

      return &tport->tport_wwn;
@@ -1443,7 +1445,7 @@ static void scsiback_drop_tport(struct se_wwn *wwn)
      struct scsiback_tport *tport = container_of(wwn,
                  struct scsiback_tport, tport_wwn);

-    pr_debug("xen-pvscsi: Deallocating emulated Target %s Address:
%s\n",
+    pr_debug("Deallocating emulated Target %s Address: %s\n",
           scsiback_dump_proto_id(tport), tport->tport_name);

      kfree(tport);
@@ -1470,8 +1472,8 @@ static u32 scsiback_tpg_get_inst_index(struct
se_portal_group *se_tpg)
  static int scsiback_check_stop_free(struct se_cmd *se_cmd)
  {
      /*
-     * Do not release struct se_cmd's containing a valid TMR
-     * pointer.  These will be released directly in
scsiback_device_action()
+     * Do not release struct se_cmd's containing a valid TMR pointer.
+     * These will be released directly in scsiback_device_action()
       * with transport_generic_free_cmd().
       */
      if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
@@ -1637,7 +1639,7 @@ static int scsiback_make_nexus(struct
scsiback_tpg *tpg,
          return -ENOMEM;
      }
      /*
-     *  Initialize the struct se_session pointer
+     * Initialize the struct se_session pointer
       */
      tv_nexus->tvn_se_sess = transport_init_session(TARGET_PROT_NORMAL);
      if (IS_ERR(tv_nexus->tvn_se_sess)) {
@@ -1708,7 +1710,7 @@ static int scsiback_drop_nexus(struct
scsiback_tpg *tpg)
          return -EBUSY;
      }

-    pr_debug("xen-pvscsi: Removing I_T Nexus to emulated %s Initiator
Port: %s\n",
+    pr_debug("Removing I_T Nexus to emulated %s Initiator Port: %s\n",
          scsiback_dump_proto_id(tpg->tport),
          tv_nexus->tvn_se_sess->se_node_acl->initiatorname);

@@ -1754,7 +1756,7 @@ static ssize_t scsiback_tpg_store_nexus(struct
se_portal_group *se_tpg,
      unsigned char i_port[VSCSI_NAMELEN], *ptr, *port_ptr;
      int ret;
      /*
-     * Shutdown the active I_T nexus if 'NULL' is passed..
+     * Shutdown the active I_T nexus if 'NULL' is passed.
       */
      if (!strncmp(page, "NULL", 4)) {
          ret = scsiback_drop_nexus(tpg);
@@ -1925,7 +1927,7 @@ static void scsiback_drop_tpg(struct
se_portal_group *se_tpg)
       */
      scsiback_drop_nexus(tpg);
      /*
-     * Deregister the se_tpg from TCM..
+     * Deregister the se_tpg from TCM.
       */
      core_tpg_deregister(se_tpg);
      kfree(tpg);
@@ -1995,7 +1997,7 @@ static int scsiback_register_configfs(void)
      struct target_fabric_configfs *fabric;
      int ret;

-    pr_debug("xen-pvscsi: fabric module %s on %s/%s on "UTS_RELEASE"\n",
+    pr_debug("fabric module %s on %s/%s on "UTS_RELEASE"\n",
           VSCSI_VERSION, utsname()->sysname, utsname()->machine);
      /*
       * Register the top level struct config_item_type with TCM core
@@ -2032,7 +2034,7 @@ static int scsiback_register_configfs(void)
       * Setup our local pointer to *fabric
       */
      scsiback_fabric_configfs = fabric;
-    pr_debug("xen-pvscsi: Set fabric -> scsiback_fabric_configfs\n");
+    pr_debug("Set fabric -> scsiback_fabric_configfs\n");
      return 0;
  };

@@ -2043,7 +2045,7 @@ static void scsiback_deregister_configfs(void)

      target_fabric_configfs_deregister(scsiback_fabric_configfs);
      scsiback_fabric_configfs = NULL;
-    pr_debug("xen-pvscsi: Cleared scsiback_fabric_configfs\n");
+    pr_debug("Cleared scsiback_fabric_configfs\n");
  };

  static const struct xenbus_device_id scsiback_ids[] = {
@@ -2094,7 +2096,7 @@ out_unregister_xenbus:
      xenbus_unregister_driver(&scsiback_driver);
  out_cache_destroy:
      kmem_cache_destroy(scsiback_cachep);
-    pr_err("xen-pvscsi: %s: error %d\n", __func__, ret);
+    pr_err("%s: error %d\n", __func__, ret);
      return ret;
  }




.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.