* [PATCH] fix: do not overwrite existing devices' symlinks @ 2012-01-23 12:06 Lukasz Dorau 2012-01-30 1:27 ` NeilBrown 0 siblings, 1 reply; 5+ messages in thread From: Lukasz Dorau @ 2012-01-23 12:06 UTC (permalink / raw) To: neilb; +Cc: linux-raid, dan.j.williams, marcin.labun, ed.ciechanowski If the device's name is given in /etc/mdadm.conf, create_mddev() does not check if the map contains a device of this name (mdopen.c:140). If it does, the symlink of that name will be overwritten. create_mddev() has been changed. Now it checks if the map contains a device of the name given in /etc/mdadm.conf. If it does, the appropriate suffix is added to the given name. Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com> --- mdopen.c | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-) diff --git a/mdopen.c b/mdopen.c index eac1c1f..3078de6 100644 --- a/mdopen.c +++ b/mdopen.c @@ -147,10 +147,12 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy, char *cname; char devname[20]; char cbuf[400]; + struct map_ent *map = NULL; + int dev_conflict = 0; + if (chosen == NULL) chosen = cbuf; - if (autof == 0) autof = ci->autof; @@ -277,17 +279,21 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy, else sprintf(devname, "/dev/md%d", num); - if (cname[0] == 0 && name) { + if ((cname[0] != 0) && map_by_name(&map, cname)) + dev_conflict = 1; + + if ((cname[0] == 0 && name) || dev_conflict) { /* Need to find a name if we can * We don't completely trust 'name'. Truncate to * reasonable length and remove '/' */ char *cp; - struct map_ent *map = NULL; int conflict = 1; int unum = 0; int cnlen; - strncpy(cname, name, 200); + + if (!dev_conflict) + strncpy(cname, name, 200); cname[200] = 0; while ((cp = strchr(cname, '/')) != NULL) *cp = '-'; @@ -312,7 +318,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy, } } - if (dev && dev[0] == '/') + if ((dev && dev[0] == '/') && (!dev_conflict)) strcpy(chosen, dev); else if (cname[0] == 0) strcpy(chosen, devname); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fix: do not overwrite existing devices' symlinks 2012-01-23 12:06 [PATCH] fix: do not overwrite existing devices' symlinks Lukasz Dorau @ 2012-01-30 1:27 ` NeilBrown 2012-01-30 12:13 ` Dorau, Lukasz 0 siblings, 1 reply; 5+ messages in thread From: NeilBrown @ 2012-01-30 1:27 UTC (permalink / raw) To: Lukasz Dorau; +Cc: linux-raid, dan.j.williams, marcin.labun, ed.ciechanowski [-- Attachment #1: Type: text/plain, Size: 2725 bytes --] On Mon, 23 Jan 2012 13:06:52 +0100 Lukasz Dorau <lukasz.dorau@intel.com> wrote: > If the device's name is given in /etc/mdadm.conf, create_mddev() > does not check if the map contains a device of this name (mdopen.c:140). > If it does, the symlink of that name will be overwritten. > > create_mddev() has been changed. Now it checks if the map contains > a device of the name given in /etc/mdadm.conf. > If it does, the appropriate suffix is added to the given name. > > Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com> Can you please remind me what the big picture problem is here?? It seem like you are suggesting that if /dev/md/thing is given in mdadm.conf, but some other array is already assembled with the name /dev/md/thing, then the array from mdadm.conf should be assembled as /dev/md/thing0 or something like that - is that correct? I don't think we want that. If there is a name conflict like this with a name given in mdadm.conf, then one of the arrays should fail to assemble as this is really a fairly serious configuration error. Or did I misunderstand? Thanks, NeilBrown > --- > mdopen.c | 16 +++++++++++----- > 1 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/mdopen.c b/mdopen.c > index eac1c1f..3078de6 100644 > --- a/mdopen.c > +++ b/mdopen.c > @@ -147,10 +147,12 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy, > char *cname; > char devname[20]; > char cbuf[400]; > + struct map_ent *map = NULL; > + int dev_conflict = 0; > + > if (chosen == NULL) > chosen = cbuf; > > - > if (autof == 0) > autof = ci->autof; > > @@ -277,17 +279,21 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy, > else > sprintf(devname, "/dev/md%d", num); > > - if (cname[0] == 0 && name) { > + if ((cname[0] != 0) && map_by_name(&map, cname)) > + dev_conflict = 1; > + > + if ((cname[0] == 0 && name) || dev_conflict) { > /* Need to find a name if we can > * We don't completely trust 'name'. Truncate to > * reasonable length and remove '/' > */ > char *cp; > - struct map_ent *map = NULL; > int conflict = 1; > int unum = 0; > int cnlen; > - strncpy(cname, name, 200); > + > + if (!dev_conflict) > + strncpy(cname, name, 200); > cname[200] = 0; > while ((cp = strchr(cname, '/')) != NULL) > *cp = '-'; > @@ -312,7 +318,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy, > } > } > > - if (dev && dev[0] == '/') > + if ((dev && dev[0] == '/') && (!dev_conflict)) > strcpy(chosen, dev); > else if (cname[0] == 0) > strcpy(chosen, devname); [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] fix: do not overwrite existing devices' symlinks 2012-01-30 1:27 ` NeilBrown @ 2012-01-30 12:13 ` Dorau, Lukasz 2012-01-30 22:13 ` NeilBrown 0 siblings, 1 reply; 5+ messages in thread From: Dorau, Lukasz @ 2012-01-30 12:13 UTC (permalink / raw) To: NeilBrown Cc: linux-raid@vger.kernel.org, Williams, Dan J, Labun, Marcin, Ciechanowski, Ed > -----Original Message----- > From: NeilBrown [mailto:neilb@suse.de] > Sent: Monday, January 30, 2012 2:27 AM > To: Dorau, Lukasz > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Labun, Marcin; Ciechanowski, > Ed > Subject: Re: [PATCH] fix: do not overwrite existing devices' symlinks > > On Mon, 23 Jan 2012 13:06:52 +0100 Lukasz Dorau <lukasz.dorau@intel.com> > wrote: > > > If the device's name is given in /etc/mdadm.conf, create_mddev() > > does not check if the map contains a device of this name (mdopen.c:140). > > If it does, the symlink of that name will be overwritten. > > > > create_mddev() has been changed. Now it checks if the map contains > > a device of the name given in /etc/mdadm.conf. > > If it does, the appropriate suffix is added to the given name. > > > > Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com> > > Can you please remind me what the big picture problem is here?? > > It seem like you are suggesting that if > /dev/md/thing > > is given in mdadm.conf, but some other array is already assembled with the > name /dev/md/thing, then the array from mdadm.conf should be assembled as > /dev/md/thing0 > or something like that - is that correct? > > I don't think we want that. If there is a name conflict like this with a > name given in mdadm.conf, then one of the arrays should fail to assemble as > this is really a fairly serious configuration error. > > Or did I misunderstand? > > -----Original Message----- > From: NeilBrown [mailto:neilb@suse.de] > Sent: Monday, January 30, 2012 2:30 AM > To: Dorau, Lukasz > Subject: Re: mdadm: checking dev's names from mdadm.conf - question > > On Fri, 20 Jan 2012 09:27:58 +0000 "Dorau, Lukasz" <lukasz.dorau@intel.com> > wrote: > > > Hi > > > > Is it OK that mdadm does not check if a symlink of the name given in > /etc/mdadm.conf already exists (in function create_mddev() in mdopen.c:140) ? > > > > For example: > > If we modify the original /etc/mdadm.conf file: > > > > ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320 > > ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320 > member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9 > > ARRAY metadata=imsm UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > > ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902 > > > > by adding the 2nd container 's name /dev/md/imsm0 to the 3rd line: > > > > ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320 > > ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320 > member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9 > > ARRAY /dev/md/imsm0 metadata=imsm > UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > > ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902 > > > > mdadm will create the first container /dev/md127 and the symlink of default > name /dev/md/imsm0 (and the first volume /dev/md126 with symlink > /dev/md/vol1). > > Later it will create the second container /dev/md125 and the symlink of the > name given in /etc/mdadm.conf - /dev/md/imsm0 - the same as the name of > the first container. > > > > Mdadm does not check if the symlink of the given name already exists and it > _overwrites_ the first symlink. Is it OK or maybe should it be corrected? > > > > Ahhh.. this is where the big-picture bit is... > > I don't have a big problem with it over-writing the symlink - that is what > you asked for in a way. > > However I also wouldn't have a problem with mdadm refusing the assemble the > second container as its name is already in use. > So, are you going to apply this patch or do you want it to be fixed in another way? Regards, Lukasz > > --- > > mdopen.c | 16 +++++++++++----- > > 1 files changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/mdopen.c b/mdopen.c > > index eac1c1f..3078de6 100644 > > --- a/mdopen.c > > +++ b/mdopen.c > > @@ -147,10 +147,12 @@ int create_mddev(char *dev, char *name, int autof, > int trustworthy, > > char *cname; > > char devname[20]; > > char cbuf[400]; > > + struct map_ent *map = NULL; > > + int dev_conflict = 0; > > + > > if (chosen == NULL) > > chosen = cbuf; > > > > - > > if (autof == 0) > > autof = ci->autof; > > > > @@ -277,17 +279,21 @@ int create_mddev(char *dev, char *name, int autof, > int trustworthy, > > else > > sprintf(devname, "/dev/md%d", num); > > > > - if (cname[0] == 0 && name) { > > + if ((cname[0] != 0) && map_by_name(&map, cname)) > > + dev_conflict = 1; > > + > > + if ((cname[0] == 0 && name) || dev_conflict) { > > /* Need to find a name if we can > > * We don't completely trust 'name'. Truncate to > > * reasonable length and remove '/' > > */ > > char *cp; > > - struct map_ent *map = NULL; > > int conflict = 1; > > int unum = 0; > > int cnlen; > > - strncpy(cname, name, 200); > > + > > + if (!dev_conflict) > > + strncpy(cname, name, 200); > > cname[200] = 0; > > while ((cp = strchr(cname, '/')) != NULL) > > *cp = '-'; > > @@ -312,7 +318,7 @@ int create_mddev(char *dev, char *name, int autof, > int trustworthy, > > } > > } > > > > - if (dev && dev[0] == '/') > > + if ((dev && dev[0] == '/') && (!dev_conflict)) > > strcpy(chosen, dev); > > else if (cname[0] == 0) > > strcpy(chosen, devname); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fix: do not overwrite existing devices' symlinks 2012-01-30 12:13 ` Dorau, Lukasz @ 2012-01-30 22:13 ` NeilBrown 2012-01-31 8:00 ` Dorau, Lukasz 0 siblings, 1 reply; 5+ messages in thread From: NeilBrown @ 2012-01-30 22:13 UTC (permalink / raw) To: Dorau, Lukasz Cc: linux-raid@vger.kernel.org, Williams, Dan J, Labun, Marcin, Ciechanowski, Ed [-- Attachment #1: Type: text/plain, Size: 4348 bytes --] On Mon, 30 Jan 2012 12:13:05 +0000 "Dorau, Lukasz" <lukasz.dorau@intel.com> wrote: > > -----Original Message----- > > From: NeilBrown [mailto:neilb@suse.de] > > Sent: Monday, January 30, 2012 2:27 AM > > To: Dorau, Lukasz > > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Labun, Marcin; Ciechanowski, > > Ed > > Subject: Re: [PATCH] fix: do not overwrite existing devices' symlinks > > > > On Mon, 23 Jan 2012 13:06:52 +0100 Lukasz Dorau <lukasz.dorau@intel.com> > > wrote: > > > > > If the device's name is given in /etc/mdadm.conf, create_mddev() > > > does not check if the map contains a device of this name (mdopen.c:140). > > > If it does, the symlink of that name will be overwritten. > > > > > > create_mddev() has been changed. Now it checks if the map contains > > > a device of the name given in /etc/mdadm.conf. > > > If it does, the appropriate suffix is added to the given name. > > > > > > Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com> > > > > Can you please remind me what the big picture problem is here?? > > > > It seem like you are suggesting that if > > /dev/md/thing > > > > is given in mdadm.conf, but some other array is already assembled with the > > name /dev/md/thing, then the array from mdadm.conf should be assembled as > > /dev/md/thing0 > > or something like that - is that correct? > > > > I don't think we want that. If there is a name conflict like this with a > > name given in mdadm.conf, then one of the arrays should fail to assemble as > > this is really a fairly serious configuration error. > > > > Or did I misunderstand? > > > > > > -----Original Message----- > > From: NeilBrown [mailto:neilb@suse.de] > > Sent: Monday, January 30, 2012 2:30 AM > > To: Dorau, Lukasz > > Subject: Re: mdadm: checking dev's names from mdadm.conf - question > > > > On Fri, 20 Jan 2012 09:27:58 +0000 "Dorau, Lukasz" <lukasz.dorau@intel.com> > > wrote: > > > > > Hi > > > > > > Is it OK that mdadm does not check if a symlink of the name given in > > /etc/mdadm.conf already exists (in function create_mddev() in mdopen.c:140) ? > > > > > > For example: > > > If we modify the original /etc/mdadm.conf file: > > > > > > ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320 > > > ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320 > > member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9 > > > ARRAY metadata=imsm UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > > > ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > > member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902 > > > > > > by adding the 2nd container 's name /dev/md/imsm0 to the 3rd line: > > > > > > ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320 > > > ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320 > > member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9 > > > ARRAY /dev/md/imsm0 metadata=imsm > > UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > > > ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > > member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902 > > > > > > mdadm will create the first container /dev/md127 and the symlink of default > > name /dev/md/imsm0 (and the first volume /dev/md126 with symlink > > /dev/md/vol1). > > > Later it will create the second container /dev/md125 and the symlink of the > > name given in /etc/mdadm.conf - /dev/md/imsm0 - the same as the name of > > the first container. > > > > > > Mdadm does not check if the symlink of the given name already exists and it > > _overwrites_ the first symlink. Is it OK or maybe should it be corrected? > > > > > > > Ahhh.. this is where the big-picture bit is... > > > > I don't have a big problem with it over-writing the symlink - that is what > > you asked for in a way. > > > > However I also wouldn't have a problem with mdadm refusing the assemble the > > second container as its name is already in use. > > > > > So, are you going to apply this patch or do you want it to be fixed in another way? The approaches I said were OK were: 1/ over-write the symlink 2/ refuse to assemble the second container The approach the patch takes is 3/ use a different name to the one in mdadm.conf As 3 != 1 and 3 != 2, I'm not going to apply the patch. NeilBrown [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] fix: do not overwrite existing devices' symlinks 2012-01-30 22:13 ` NeilBrown @ 2012-01-31 8:00 ` Dorau, Lukasz 0 siblings, 0 replies; 5+ messages in thread From: Dorau, Lukasz @ 2012-01-31 8:00 UTC (permalink / raw) To: NeilBrown Cc: linux-raid@vger.kernel.org, Williams, Dan J, Labun, Marcin, Ciechanowski, Ed > -----Original Message----- > From: NeilBrown [mailto:neilb@suse.de] > Sent: Monday, January 30, 2012 11:14 PM > To: Dorau, Lukasz > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Labun, Marcin; Ciechanowski, > Ed > Subject: Re: [PATCH] fix: do not overwrite existing devices' symlinks > > On Mon, 30 Jan 2012 12:13:05 +0000 "Dorau, Lukasz" > <lukasz.dorau@intel.com> > wrote: > > > > -----Original Message----- > > > From: NeilBrown [mailto:neilb@suse.de] > > > Sent: Monday, January 30, 2012 2:27 AM > > > To: Dorau, Lukasz > > > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Labun, Marcin; > Ciechanowski, > > > Ed > > > Subject: Re: [PATCH] fix: do not overwrite existing devices' symlinks > > > > > > On Mon, 23 Jan 2012 13:06:52 +0100 Lukasz Dorau > <lukasz.dorau@intel.com> > > > wrote: > > > > > > > If the device's name is given in /etc/mdadm.conf, create_mddev() > > > > does not check if the map contains a device of this name (mdopen.c:140). > > > > If it does, the symlink of that name will be overwritten. > > > > > > > > create_mddev() has been changed. Now it checks if the map contains > > > > a device of the name given in /etc/mdadm.conf. > > > > If it does, the appropriate suffix is added to the given name. > > > > > > > > Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com> > > > > > > Can you please remind me what the big picture problem is here?? > > > > > > It seem like you are suggesting that if > > > /dev/md/thing > > > > > > is given in mdadm.conf, but some other array is already assembled with the > > > name /dev/md/thing, then the array from mdadm.conf should be assembled > as > > > /dev/md/thing0 > > > or something like that - is that correct? > > > > > > I don't think we want that. If there is a name conflict like this with a > > > name given in mdadm.conf, then one of the arrays should fail to assemble as > > > this is really a fairly serious configuration error. > > > > > > Or did I misunderstand? > > > > > > > > > > -----Original Message----- > > > From: NeilBrown [mailto:neilb@suse.de] > > > Sent: Monday, January 30, 2012 2:30 AM > > > To: Dorau, Lukasz > > > Subject: Re: mdadm: checking dev's names from mdadm.conf - question > > > > > > On Fri, 20 Jan 2012 09:27:58 +0000 "Dorau, Lukasz" > <lukasz.dorau@intel.com> > > > wrote: > > > > > > > Hi > > > > > > > > Is it OK that mdadm does not check if a symlink of the name given in > > > /etc/mdadm.conf already exists (in function create_mddev() in > mdopen.c:140) ? > > > > > > > > For example: > > > > If we modify the original /etc/mdadm.conf file: > > > > > > > > ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320 > > > > ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320 > > > member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9 > > > > ARRAY metadata=imsm UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > > > > ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > > > member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902 > > > > > > > > by adding the 2nd container 's name /dev/md/imsm0 to the 3rd line: > > > > > > > > ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320 > > > > ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320 > > > member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9 > > > > ARRAY /dev/md/imsm0 metadata=imsm > > > UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > > > > ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e > > > member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902 > > > > > > > > mdadm will create the first container /dev/md127 and the symlink of > default > > > name /dev/md/imsm0 (and the first volume /dev/md126 with symlink > > > /dev/md/vol1). > > > > Later it will create the second container /dev/md125 and the symlink of the > > > name given in /etc/mdadm.conf - /dev/md/imsm0 - the same as the name of > > > the first container. > > > > > > > > Mdadm does not check if the symlink of the given name already exists and > it > > > _overwrites_ the first symlink. Is it OK or maybe should it be corrected? > > > > > > > > > > Ahhh.. this is where the big-picture bit is... > > > > > > I don't have a big problem with it over-writing the symlink - that is what > > > you asked for in a way. > > > > > > However I also wouldn't have a problem with mdadm refusing the assemble > the > > > second container as its name is already in use. > > > > > > > > > So, are you going to apply this patch or do you want it to be fixed in another > way? > > The approaches I said were OK were: > 1/ over-write the symlink > 2/ refuse to assemble the second container > > The approach the patch takes is > 3/ use a different name to the one in mdadm.conf > > As 3 != 1 and 3 != 2, I'm not going to apply the patch. > OK, I had misunderstood you previously. Regards, Lukasz ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-01-31 8:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-01-23 12:06 [PATCH] fix: do not overwrite existing devices' symlinks Lukasz Dorau 2012-01-30 1:27 ` NeilBrown 2012-01-30 12:13 ` Dorau, Lukasz 2012-01-30 22:13 ` NeilBrown 2012-01-31 8:00 ` Dorau, Lukasz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).